Skip to content

Commit ddd1eae

Browse files
bsosnaderBrenden SosnaderCopilot
authored
Fix WeaklyCanonicalPath ERROR_ACCESS_DENIED in Windows AppContainers (microsoft#28509)
### Description Adds a Windows-only fallback to `WeaklyCanonicalPath` (in `onnxruntime/core/framework/tensorprotoutils.cc`) for use inside Windows AppContainers, where `std::filesystem::weakly_canonical` always fails with `ERROR_ACCESS_DENIED` because the underlying `GetFinalPathNameByHandleW(VOLUME_NAME_DOS)` call goes through the Volume Mount Manager, which AppContainer tokens cannot query regardless of file ACL grants. On `ERROR_ACCESS_DENIED`, fall back to a manual canonicalization that uses `GetFinalPathNameByHandleW(FILE_NAME_NORMALIZED | VOLUME_NAME_NT)` and prefixes the result with `\\?\GLOBALROOT` so it remains a valid Win32 path. All other error paths, non-Windows builds, and non-AppContainer Windows runs are unchanged. `VOLUME_NAME_NT` (not `VOLUME_NAME_NONE`) is required: it preserves volume identity, so the cross-volume escape rejection in `ValidateExternalDataPath` introduced by microsoft#26776 continues to hold. 8 new unit tests cover the fallback helper directly (existing dir/file, non-existent leaf, multi-component miss, all-non-existent → false, equivalence with `weakly_canonical`, symlink resolution, `..` collapse). The AppContainer trigger itself cannot be reproduced in a unit test environment. ### Motivation and Context Fixes microsoft#28508. Regression introduced in v1.24.1 by microsoft#26776 (`ValidateExternalDataPath`); current `WeaklyCanonicalPath` wrapper from microsoft#27539 in v1.25.0. Loading any ONNX model with external data fails inside a Windows AppContainer with: ``` Failed to get the weakly canonical path: "<path>" - Access is denied. ``` Affected callers have no in-process workaround. Downstream report: microsoft/foundry-local#709. CC @yuslepukhin (microsoft#26776), @adrianlizarraga (microsoft#27539), @tianleiwu (microsoft#27374). --------- Co-authored-by: Brenden Sosnader <brsosnad@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 127faf0 commit ddd1eae

6 files changed

Lines changed: 282 additions & 4 deletions

File tree

onnxruntime/core/framework/tensorprotoutils.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -380,11 +380,11 @@ Status TensorProtoWithExternalDataToTensorProto(
380380
return Status::OK();
381381
}
382382

383-
// Wraps std::filesystem::weakly_canonical with error_code handling.
383+
// Wraps Env::GetWeaklyCanonicalPath for std::filesystem::path.
384384
static Status WeaklyCanonicalPath(const std::filesystem::path& path, std::filesystem::path& result) {
385-
std::error_code ec;
386-
result = std::filesystem::weakly_canonical(path, ec);
387-
ORT_RETURN_IF(ec, "Failed to get the weakly canonical path: ", path, " - ", ec.message());
385+
PathString canonical_str;
386+
ORT_RETURN_IF_ERROR(Env::Default().GetWeaklyCanonicalPath(path.native(), canonical_str));
387+
result = std::filesystem::path(std::move(canonical_str));
388388
return Status::OK();
389389
}
390390

onnxruntime/core/platform/env.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,12 @@ class Env {
231231
const PathString& path,
232232
PathString& canonical_path) const = 0;
233233

234+
/** Like GetCanonicalPath, but the path is not required to exist. Mirrors
235+
* std::filesystem::weakly_canonical. */
236+
virtual common::Status GetWeaklyCanonicalPath(
237+
const PathString& path,
238+
PathString& canonical_path) const = 0;
239+
234240
// This functions is always successful. It can't fail.
235241
virtual PIDType GetSelfPid() const = 0;
236242

onnxruntime/core/platform/posix/env.cc

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,19 @@ class PosixEnv : public Env {
543543
return Status::OK();
544544
}
545545

546+
common::Status GetWeaklyCanonicalPath(
547+
const PathString& path,
548+
PathString& canonical_path) const override {
549+
std::error_code ec;
550+
auto canonical = std::filesystem::weakly_canonical(std::filesystem::path{path}, ec);
551+
if (ec) {
552+
return common::Status(common::ONNXRUNTIME, common::FAIL,
553+
"Failed to get the weakly canonical path: " + path + " - " + ec.message());
554+
}
555+
canonical_path.assign(canonical.native());
556+
return Status::OK();
557+
}
558+
546559
common::Status LoadDynamicLibrary(const PathString& library_filename, bool global_symbols, void** handle) const override {
547560
dlerror(); // clear any old error_str
548561
*handle = dlopen(library_filename.c_str(), RTLD_NOW | (global_symbols ? RTLD_GLOBAL : RTLD_LOCAL));

onnxruntime/core/platform/windows/env.cc

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ limitations under the License.
2020

2121
#include <iostream>
2222
#include <fstream>
23+
#include <filesystem>
2324
#include <optional>
2425
#include <string>
2526
#include <thread>
@@ -685,6 +686,136 @@ common::Status WindowsEnv::GetCanonicalPath(
685686
return Status::OK();
686687
}
687688

689+
namespace {
690+
691+
constexpr std::wstring_view kGlobalRootPrefix{L"\\\\?\\GLOBALROOT"};
692+
693+
wil::unique_hfile OpenHandleForFinalPath(const std::filesystem::path& path) {
694+
CREATEFILE2_EXTENDED_PARAMETERS params{};
695+
params.dwSize = sizeof(params);
696+
params.dwFileFlags = FILE_FLAG_BACKUP_SEMANTICS;
697+
return wil::unique_hfile{::CreateFile2(path.c_str(),
698+
FILE_READ_ATTRIBUTES,
699+
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
700+
OPEN_EXISTING,
701+
&params)};
702+
}
703+
704+
// Final-path query using VOLUME_NAME_NT, prefixed with "\\?\GLOBALROOT" to stay a valid Win32 path.
705+
bool TryGetFinalPathNt(const std::filesystem::path& path, std::filesystem::path& result) {
706+
wil::unique_hfile handle = OpenHandleForFinalPath(path);
707+
if (handle.get() == INVALID_HANDLE_VALUE) {
708+
return false;
709+
}
710+
711+
std::wstring buffer(MAX_PATH, L'\0');
712+
constexpr DWORD kFlags = FILE_NAME_NORMALIZED | VOLUME_NAME_NT;
713+
DWORD needed = ::GetFinalPathNameByHandleW(handle.get(), buffer.data(),
714+
static_cast<DWORD>(buffer.size()), kFlags);
715+
if (needed != 0 && needed >= buffer.size()) {
716+
buffer.resize(needed);
717+
needed = ::GetFinalPathNameByHandleW(handle.get(), buffer.data(),
718+
static_cast<DWORD>(buffer.size()), kFlags);
719+
}
720+
721+
if (needed == 0 || needed >= buffer.size()) {
722+
return false;
723+
}
724+
buffer.resize(needed);
725+
726+
std::wstring prefixed;
727+
prefixed.reserve(kGlobalRootPrefix.size() + buffer.size());
728+
prefixed.append(kGlobalRootPrefix);
729+
prefixed.append(buffer);
730+
result = std::filesystem::path(std::move(prefixed));
731+
return true;
732+
}
733+
734+
// weakly_canonical analogue using TryGetFinalPathNt for the existing prefix.
735+
bool TryWeaklyCanonicalPathNtVolume(const std::filesystem::path& input,
736+
std::filesystem::path& result) {
737+
std::filesystem::path head = input;
738+
std::filesystem::path tail;
739+
std::filesystem::path canonical_head;
740+
bool found_existing_prefix = false;
741+
742+
while (true) {
743+
std::error_code ec;
744+
const bool exists = std::filesystem::exists(head, ec);
745+
if (ec) {
746+
return false;
747+
}
748+
if (exists) {
749+
if (!TryGetFinalPathNt(head, canonical_head)) {
750+
return false;
751+
}
752+
found_existing_prefix = true;
753+
break;
754+
}
755+
if (head.empty()) {
756+
break;
757+
}
758+
const auto parent = head.parent_path();
759+
if (parent == head) {
760+
break;
761+
}
762+
const auto leaf = head.filename();
763+
if (!leaf.empty()) {
764+
// path / empty would insert a trailing separator.
765+
tail = tail.empty() ? leaf : (leaf / tail);
766+
}
767+
head = parent;
768+
}
769+
770+
if (!found_existing_prefix) {
771+
return false;
772+
}
773+
774+
if (tail.empty()) {
775+
result = std::move(canonical_head);
776+
} else {
777+
result = (canonical_head / tail).lexically_normal();
778+
}
779+
return true;
780+
}
781+
782+
} // namespace
783+
784+
// On AppContainer, std::filesystem::weakly_canonical fails with ERROR_ACCESS_DENIED
785+
// because VOLUME_NAME_DOS goes through the Volume Mount Manager. Fall back to
786+
// VOLUME_NAME_NT, which preserves volume identity (cross-volume escape rejection in
787+
// ValidateExternalDataPath relies on this — do NOT use VOLUME_NAME_NONE).
788+
common::Status WindowsEnv::GetWeaklyCanonicalPath(
789+
const PathString& path,
790+
PathString& canonical_path) const {
791+
std::filesystem::path fs_path{path};
792+
std::error_code ec;
793+
std::filesystem::path canonical = std::filesystem::weakly_canonical(fs_path, ec);
794+
if (!ec) {
795+
canonical_path = canonical.native();
796+
return Status::OK();
797+
}
798+
799+
if (ec.value() == ERROR_ACCESS_DENIED) {
800+
std::filesystem::path fallback;
801+
if (TryWeaklyCanonicalPathNtVolume(fs_path, fallback)) {
802+
canonical_path = fallback.native();
803+
return Status::OK();
804+
}
805+
}
806+
807+
return ORT_MAKE_STATUS(ONNXRUNTIME, FAIL,
808+
"Failed to get the weakly canonical path: ",
809+
ToUTF8String(path), " - ", ec.message());
810+
}
811+
812+
namespace internal {
813+
bool WeaklyCanonicalPathNtVolumeFallbackForTesting(const std::filesystem::path& input,
814+
std::filesystem::path& result) {
815+
return TryWeaklyCanonicalPathNtVolume(input, result);
816+
}
817+
} // namespace internal
818+
688819
// Return the path of the executable/shared library for the current running code. This is to make it
689820
// possible to load other shared libraries installed next to our core runtime code.
690821
PathString WindowsEnv::GetRuntimePath() const {

onnxruntime/core/platform/windows/env.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ limitations under the License.
1818
#include "core/platform/windows/telemetry.h"
1919
#include "core/common/inlined_containers.h"
2020
#include <Windows.h>
21+
#include <filesystem>
2122

2223
namespace onnxruntime {
2324

@@ -79,6 +80,7 @@ class WindowsEnv : public Env {
7980
common::Status FileOpenWr(const std::string& path, /*out*/ int& fd) const override;
8081
common::Status FileClose(int fd) const override;
8182
common::Status GetCanonicalPath(const PathString& path, PathString& canonical_path) const override;
83+
common::Status GetWeaklyCanonicalPath(const PathString& path, PathString& canonical_path) const override;
8284
PathString GetRuntimePath() const override;
8385
Status LoadDynamicLibrary(const PathString& library_filename, bool /*global_symbols*/, void** handle) const override;
8486
Status UnloadDynamicLibrary(void* handle) const override;
@@ -141,4 +143,10 @@ class WindowsEnv : public Env {
141143
WindowsTelemetry telemetry_provider_;
142144
};
143145

146+
namespace internal {
147+
// Test-only: exposes the AppContainer fallback used by WindowsEnv::GetWeaklyCanonicalPath.
148+
bool WeaklyCanonicalPathNtVolumeFallbackForTesting(const std::filesystem::path& input,
149+
std::filesystem::path& result);
150+
} // namespace internal
151+
144152
} // namespace onnxruntime

onnxruntime/test/framework/tensorutils_test.cc

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#ifdef _WIN32
2323
#include <Windows.h>
24+
#include "core/platform/windows/env.h"
2425
#endif
2526

2627
using namespace ::onnxruntime::utils;
@@ -715,6 +716,125 @@ TEST_F(PathValidationTest, ValidateExternalDataPathEmptyModelPathWithSymlinkOuts
715716
EXPECT_THAT(status.ErrorMessage(), testing::HasSubstr("escapes working directory"));
716717
}
717718

719+
#if defined(_WIN32)
720+
// Direct tests for the Windows AppContainer fallback used by
721+
// WindowsEnv::GetWeaklyCanonicalPath. The AppContainer trigger itself can't be
722+
// reproduced in a unit test environment; see microsoft/onnxruntime#28508.
723+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_ExistingDirectory) {
724+
std::filesystem::path canonical;
725+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(base_dir_, canonical));
726+
727+
EXPECT_THAT(canonical.wstring(), testing::StartsWith(L"\\\\?\\GLOBALROOT\\Device\\"));
728+
729+
std::error_code ec;
730+
EXPECT_TRUE(std::filesystem::exists(canonical, ec)) << "ec=" << ec.message();
731+
}
732+
733+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_ExistingFile) {
734+
CreateEmptyFile(base_dir_ / "data.bin");
735+
736+
std::filesystem::path canonical;
737+
ASSERT_TRUE(
738+
onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(base_dir_ / "data.bin", canonical));
739+
740+
EXPECT_THAT(canonical.wstring(), testing::StartsWith(L"\\\\?\\GLOBALROOT\\Device\\"));
741+
742+
std::error_code ec;
743+
EXPECT_TRUE(std::filesystem::exists(canonical, ec)) << "ec=" << ec.message();
744+
EXPECT_TRUE(std::filesystem::is_regular_file(canonical, ec)) << "ec=" << ec.message();
745+
}
746+
747+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_NonExistentLeafLexicallyAppended) {
748+
const std::filesystem::path leaf{L"does_not_exist.bin"};
749+
std::filesystem::path canonical;
750+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(base_dir_ / leaf, canonical));
751+
752+
EXPECT_THAT(canonical.wstring(), testing::StartsWith(L"\\\\?\\GLOBALROOT\\Device\\"));
753+
EXPECT_EQ(canonical.filename(), leaf);
754+
755+
// The canonicalized parent must be a path-component prefix of the result so that the
756+
// containment check in ValidateExternalDataPath still works.
757+
std::filesystem::path parent_canonical;
758+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(base_dir_, parent_canonical));
759+
auto [parent_end, full_it] = std::mismatch(parent_canonical.begin(), parent_canonical.end(),
760+
canonical.begin(), canonical.end());
761+
EXPECT_EQ(parent_end, parent_canonical.end())
762+
<< "parent: " << parent_canonical << " full: " << canonical;
763+
}
764+
765+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_NonExistentMiddleAndLeaf) {
766+
std::filesystem::path canonical;
767+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(
768+
base_dir_ / L"missing_dir" / L"data.bin", canonical));
769+
770+
EXPECT_THAT(canonical.wstring(), testing::StartsWith(L"\\\\?\\GLOBALROOT\\Device\\"));
771+
EXPECT_EQ(canonical.filename(), std::filesystem::path{L"data.bin"});
772+
EXPECT_EQ(canonical.parent_path().filename(), std::filesystem::path{L"missing_dir"});
773+
}
774+
775+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_AllNonExistentReturnsFalse) {
776+
// Synthetic absolute path on a non-existent volume. The fallback must return false so
777+
// the caller surfaces the original weakly_canonical error rather than substituting an
778+
// unverified path.
779+
const std::filesystem::path bogus{L"\\\\?\\Volume{00000000-0000-0000-0000-000000000000}\\nope\\data.bin"};
780+
std::filesystem::path canonical;
781+
EXPECT_FALSE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(bogus, canonical));
782+
}
783+
784+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_MatchesWeaklyCanonicalAtFile) {
785+
// Compare via std::filesystem::equivalent: the fallback returns the NT form
786+
// (\\?\GLOBALROOT\Device\HarddiskVolumeN\...) while weakly_canonical returns the DOS
787+
// form (C:\...), but both must point at the same file.
788+
CreateEmptyFile(base_dir_ / "compare.bin");
789+
const auto target = base_dir_ / "compare.bin";
790+
791+
std::error_code ec;
792+
const auto reference = std::filesystem::weakly_canonical(target, ec);
793+
ASSERT_FALSE(ec) << ec.message();
794+
795+
std::filesystem::path fallback;
796+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(target, fallback));
797+
798+
EXPECT_TRUE(std::filesystem::equivalent(reference, fallback, ec))
799+
<< "reference=" << reference << " fallback=" << fallback << " ec=" << ec.message();
800+
}
801+
802+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_ResolvesSymlinks) {
803+
const auto target = base_dir_ / "symlink_target.bin";
804+
const auto link = base_dir_ / "symlink_link.bin";
805+
try {
806+
std::ofstream{target};
807+
std::filesystem::create_symlink(target, link);
808+
} catch (const std::exception& e) {
809+
GTEST_SKIP() << "Symlink creation not supported in this environment: " << e.what();
810+
}
811+
812+
std::filesystem::path link_canonical;
813+
std::filesystem::path target_canonical;
814+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(link, link_canonical));
815+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(target, target_canonical));
816+
817+
std::error_code ec;
818+
EXPECT_TRUE(std::filesystem::equivalent(link_canonical, target_canonical, ec))
819+
<< "link=" << link_canonical << " target=" << target_canonical << " ec=" << ec.message();
820+
}
821+
822+
TEST_F(PathValidationTest, WeaklyCanonicalPathNtVolumeFallback_ResolvesDotDot) {
823+
CreateDirectories(base_dir_ / "sub_for_dotdot");
824+
825+
std::filesystem::path canonical;
826+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(
827+
base_dir_ / "sub_for_dotdot" / "..", canonical));
828+
829+
std::filesystem::path base_canonical;
830+
ASSERT_TRUE(onnxruntime::internal::WeaklyCanonicalPathNtVolumeFallbackForTesting(base_dir_, base_canonical));
831+
832+
std::error_code ec;
833+
EXPECT_TRUE(std::filesystem::equivalent(canonical, base_canonical, ec))
834+
<< "canonical=" << canonical << " base=" << base_canonical << " ec=" << ec.message();
835+
}
836+
#endif // defined(_WIN32)
837+
718838
TEST(TensorProtoUtilsTest, GetNodeProtoLayeringAnnotation) {
719839
// Case 1: Annotation exists
720840
{

0 commit comments

Comments
 (0)