[Akshay Sonawane (apsonawane)]

This pull request enhances the security and robustness of external data file handling in ONNX Runtime by improving how external files are accessed, validated, and mapped into memory. The changes introduce canonical path and memory-mapping support to the RandomAccessFile abstraction, update file access logic to use open file handles rather than paths, and add stricter validation to prevent directory traversal attacks. Additionally, platform-specific implementations and tests have been updated to support these improvements.

Security and Path Validation Enhancements:

• Added ValidateOpenedExternalDataPath to ensure that opened external data files do not escape the model directory, mitigating directory traversal vulnerabilities. This validation is now called after opening external data files and before reading or mapping their contents. [1] [2] [3]

RandomAccessFile API Extensions:

• Extended the RandomAccessFile interface with GetCanonicalPath and Map methods, enabling platform-independent retrieval of the canonical file path and memory-mapping of file regions.
• Implemented these methods for POSIX (PosixRandomAccessFile) and Windows (WindowsRandomAccessFile) platforms, including robust error handling and platform-specific path normalization. [1] [2]

Refactoring of File Access Logic:

• Updated all external data file reading and prepacked weight loading logic to use the RandomAccessFile abstraction for reads and memory mapping, instead of repeatedly opening files by path. This ensures consistency and prevents issues if the file path is changed after opening. [1] [2] [3] [4] [5] [6] [7]

Testing Improvements:

• Added a new test to verify that memory mapping and canonical path retrieval operate correctly even if the file path is replaced after the file is opened, ensuring the API behaves as documented.

Platform and Build Enhancements:

• Included necessary headers and improved platform compatibility for new features, such as memory mapping and canonical path retrieval.

These changes collectively make external data handling more secure, reliable, and portable across platforms.

[Copilot]

Copilot review overview

🟡 Changes recommended

AppContainer and native POSIX regressions remain, and device-loader reads still bypass handle-bound validation.

Review effort: Balanced
Findings: 3 · 1

Open (4)

• Validate CUDA external-data loads against symlink swaps · New
• Support race-safe canonicalization on all supported POSIX targets · New
• Retry NT volume naming after AppContainer access denial · New
• Verify canonical path matches the opened file · New

What changed in this PR

Strengthens external-data handling by binding validation and reads to opened file handles.

Changes:

• Extends RandomAccessFile with canonical-path and memory-mapping APIs.
• Adds POSIX and Windows implementations.
• Validates opened external files and tests path-replacement behavior.

File	Description
onnxruntime/​core/​platform/​env.h	Extends the random-access file interface.
onnxruntime/​core/​platform/​posix/​env.cc	Implements POSIX canonical paths and mapping.
onnxruntime/​core/​platform/​windows/​env.cc	Implements Windows canonical paths and mapping.
onnxruntime/​core/​framework/​tensorprotoutils.cc	Uses opened handles for validation and loading.
onnxruntime/​test/​platform/​env_test.cc	Tests mapping after pathname replacement.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

You can commit the suggested changes from lintrunner.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 2 · 1

Open (4)

• PosixRandomAccessFile::Map requests PROT_WRITE for the mapping. For typical read-only file… · New
• MappedMemoryPtr uses std::function as the deleter type, which can introduce heap… · New
• sysconf(_SC_PAGESIZE) can return -1 on error. Converting that to size_t (and then using it as… · New
• This trailing comment is incorrect/misleading: it closes the IExternalDataLoader class, not the… · New

Resolved since last review (4)

• Retry NT volume naming after AppContainer access denial
• Support race-safe canonicalization on all supported POSIX targets
• Validate CUDA external-data loads against symlink swaps
• Verify canonical path matches the opened file

[Ti-Tai Wang (titaiwangms)]

Binding validation and reads to one opened file is a useful improvement, but the new CPU mapping path introduces portability regressions and the test loader no longer matches the retained WASM interface. Please address the three inline findings. These findings are grounded in the pinned source/platform contracts; they were not locally runtime-tested, and the WASM checks were still queued when inspected.

Review exclusions/open questions: accessible procfs is now required by the Linux canonical-path query; the fallback canonicalization and Windows long-path/UNC/AppContainer behavior need platform coverage. In-place filesystem mutation remains outside the abstraction's documented guarantees. No exhaustive security audit or performance claim is implied.

[Tianlei Wu (tianleiwu)]

The same-opened-object map/read/provider routing is an improvement, and the earlier CUDA-loader bypass is fixed. The generic POSIX fallback still does not establish a race-safe canonical path; I continued the portability thread with the concrete symlink interleaving. The existing read-only big-endian mapping and 32-bit backing-file concerns remain open, and current WASM Debug compiler evidence confirms the existing test-signature blocker. One additional Linux deployment regression is inline. Reviewed all 12 changed files and nearby platform/loader consumers; no local execution. Other pre-existing mutable-root/typed-unpack hardening gaps are not claimed as new regressions.

[Ti-Tai Wang (titaiwangms)]

All three previous findings are addressed in source: POSIX mappings are private/writable for byte swapping, file lengths/range metadata use uint64_t, and the tracking loader has matching WASM/native signatures. One remaining platform-test blocker becomes explicit with the new fail-closed canonicalization fallback: the new opened-file mapping test is still unconditional on WASM, where GetCanonicalPath is unsupported.

This finding is grounded in the pinned test, platform implementation, CMake test selection, and a passive clang Emscripten-target macro probe; no local ORT/WASM test was run and the current WASM job is still pending. Linux still intentionally requires procfs, and unimplemented POSIX canonicalization now fails closed; runtime coverage for big-endian, Win32 large backing files, and Windows path variants remains excluded.

[Tianlei Wu (tianleiwu)]

Re-reviewed all 13 changed files at aaee5dc. No remaining actionable findings. The loading guide now explicitly documents the native POSIX fail-closed restriction and Linux/Android procfs requirement, and the mapping test excludes native canonicalization under WASM. The same-opened-file CPU/prepacked/CUDA routing, wide length metadata, writable private POSIX mapping, and Windows AppContainer fallback address the earlier concerns. Source review only; no local build/runtime verification. WASM Release static-library and React Native iOS E2E checks were still pending at review time; please let CI complete before merging.

[Tianlei Wu (tianleiwu)]

Re-reviewed the current head. The merge of main does not change the PR's own files, and all earlier review threads are resolved. I found no remaining correctness or security concern in the handle-bound validation (open, canonical path, containment check, then read or map on the same handle).

One CI item remains: AndroidBinarySizeCheckJob_MinimalBaseline fails on this head (Sections total size ... of 1604334 exceeds threshold of 1601536 by 2798). The same job passed at aaee5dce, so the PR now consumes the remaining headroom on top of current main. Please either reduce the added code in the minimal baseline (see the inline suggestion) or raise BINARY_SIZE_THRESHOLD_IN_BYTES in .github/workflows/android.yml with a note that the growth comes from the opened-file validation. build_x64_release and webgpu_compile_x64_RelWithDebInfo (vcpkg) were still running when I checked, so please confirm they finish green.

[Ti-Tai Wang (titaiwangms)]

The previous WASM test blocker is addressed: the native GetCanonicalPath assertion is now excluded on WASM while the mapping/read coverage remains. Reviewed the shared OpenValidatedExternalDataFile extraction at all three native entry points; it retains validation of the opened handle before access. The new documentation explicitly states procfs and unsupported-POSIX fail-closed requirements. No actionable introduced blocker found in this revision.

Required CI is not yet green: the current-head Android minimal size check measured 1,604,278 bytes against a 1,601,536-byte threshold (job 111969099912), a 2,742-byte excess. Please resolve the size gate with measured justification; this approval does not waive it. Other checks remain pending. Static incremental review; no local tests, big-endian/Win32 runtime checks, or exhaustive filesystem-race testing.