EPContext sample helper follow-ups + compiled-model encryption design doc#29294
EPContext sample helper follow-ups + compiled-model encryption design doc#29294GopalakrishnanN wants to merge 8 commits into
Conversation
IsDirectoryOrEmptyName now also rejects a leaf .. (e.g. sub/..) so untrusted model-derived directory-like names are rejected up front instead of failing later as a confusing file I/O error. Non-leaf .. stays handled by ContainsPathTraversal and the model-directory containment check. Add a move-only EpContextData owning buffer plus ReadEpContextData overloads: the app read-callback path now adopts the allocator-provided buffer instead of copying it into a std::vector, and the file path reads straight into the owned vector. The std::vector ReadEpContextDataWithFileFallback overloads remain as convenience wrappers. The example EP uses the zero-copy path. Adds a zero-copy unit test and a sub/.. rejection test.
…h resolution Addresses #28624 review feedback. Now that IsResolvedPathWithinBase() does real model-directory containment for untrusted model-relative names, the lexical ContainsPathTraversal() guard is no longer applied on the trusted graph==nullptr branch of ResolveEpContextDataPath(): trusted callers already may pass absolute paths and own their paths, so there is no model directory to contain against. ContainsPathTraversal() is kept solely for ValidateEpContextDataName(), which validates the logical callback-namespace name written into the model ep_cache_context attribute (never resolved against a filesystem base). Updates the two trusted-branch tests; logical-name and model-directory containment coverage is unchanged.
abe505c to
4369676
Compare
There was a problem hiding this comment.
Pull request overview
Follow-up improvements to the AutoEP sample EPContext data helper (ep_context_data_utils.h) to tighten untrusted-name validation, reduce read-path copying by adopting callback-allocated buffers, and relax trusted-path traversal guarding where no model-directory containment applies.
Changes:
- Reject directory-like model-derived EPContext names including a leaf
..(e.g.,sub/..) earlier during path resolution. - Introduce move-only
EpContextDataand newReadEpContextData(...)overloads to enable zero-copy adoption of callback-provided buffers. - Update the example EP and unit tests to use/cover the new read API and trusted-path semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| onnxruntime/test/autoep/library/example_plugin_ep/ep.cc | Switches sample EP to the new zero-copy ReadEpContextData API. |
| onnxruntime/test/autoep/library/ep_context_data_utils.h | Adds EpContextData + new read overloads; adjusts traversal/directory validation behavior and related comments. |
| onnxruntime/test/autoep/ep_context_data_utils_test.cc | Updates resolver expectations and adds a new test covering zero-copy adoption + file fallback. |
Clear the vector-output convenience wrapper before invoking the callback reader and avoid pointer arithmetic for empty EpContextData buffers by assigning only when non-empty. Update traversal comments to describe model-relative non-leaf .. as canonicalized and containment-checked rather than always rejected. Add focused tests for empty callback payloads and stale-output clearing on callback failure.
Addresses #29294 review feedback. ReadEpContextData() now holds any callback-allocated buffer in a local RAII guard and transfers ownership into the EpContextData out-param only after the callback status and (buffer, size) validation pass. On any error path the guard frees the buffer and out stays empty, matching the reset-first / bytes-on-success contract and the std::vector overload empty-on-failure guarantee. Adds a regression test with a callback that allocates then fails.
Addresses #29294 review feedback. The std::vector<char> ReadEpContextDataWithFileFallback overload now clears data at the very top, before the file_name null/empty check, so data is empty on every error path - matching the reset-first / empty-on-failure contract of the EpContextData overload. Adds an assertion that a stale vector is cleared when file_name is empty.
.. leaf names and zero-copy read| // Use the C allocator API (not Ort::AllocatorWithDefaultOptions, whose constructor throws) so this OrtStatus*-based | ||
| // helper stays exception-free. The default allocator is owned by ORT and must not be released here. | ||
| OrtAllocator* allocator = nullptr; | ||
| RETURN_IF_ERROR(api.GetAllocatorWithDefaultOptions(&allocator)); |
There was a problem hiding this comment.
do we want to allow a user to pass in their own allocator instead of always using the default one?
There was a problem hiding this comment.
Implemented in bdec6b7. ReadEpContextData now takes an optional trailing OrtAllocator* (default nullptr = ORT default allocator). When supplied, that allocator is handed to the read callback for the output buffer and stored in EpContextData, so the matching free uses the same allocator - alloc and free stay on one allocator. Existing callers are unaffected (defaulted arg). Added a MockedOrtAllocator test asserting the caller allocator is used for the callback buffer and that the buffer is freed via that same allocator (no leak). Let me know if you would rather keep it default-only.
There was a problem hiding this comment.
now that we do allow a user-specified allocator, perhaps we should use it for the file-read case as well instead of std::vector's heap allocation. if a user is providing an allocator, they might reasonably expect that it is used consistently.
Break the Load current-gap bullet into the plaintext-on-disk and double-buffering sub-problems and note how the read callback (allocate-once via the ORT allocator) closes both.
…ContextData Addresses #29294 review feedback. ReadEpContextData now takes an optional OrtAllocator* (default nullptr = ORT default allocator). The supplied allocator is handed to the read callback for the output buffer and stored in EpContextData so the matching free uses the same allocator. Adds a MockedOrtAllocator-based test verifying the caller allocator is used and the buffer is freed via it.
| // Use the C allocator API (not Ort::AllocatorWithDefaultOptions, whose constructor throws) so this OrtStatus*-based | ||
| // helper stays exception-free. The default allocator is owned by ORT and must not be released here. | ||
| OrtAllocator* allocator = nullptr; | ||
| RETURN_IF_ERROR(api.GetAllocatorWithDefaultOptions(&allocator)); |
There was a problem hiding this comment.
now that we do allow a user-specified allocator, perhaps we should use it for the file-read case as well instead of std::vector's heap allocation. if a user is providing an allocator, they might reasonably expect that it is used consistently.
| // OrtWriteNamedBufferFunc (carried by OrtEpContextConfig) and fall back to file I/O when no callback is configured. | ||
| // The other functions are lower-level building blocks. Production EPs should additionally apply their own sandboxing, | ||
| // size limits, and path policies; see the per-function notes on how untrusted, model-derived names are treated. | ||
| // The intended entry points for EP implementers are the ReadEpContextData / WriteEpContextDataWithFileFallback |
There was a problem hiding this comment.
nit: can we document the intended entry points with Doxygen-style documentation comments (e.g., like the C API functions)?
|
|
||
| // Zero-copy read: reads EPContext binary data named `file_name` into `out` (reset first). If `read_func` is non-null | ||
| // it is invoked and the buffer it allocates is adopted by `out` (no copy); otherwise the data is read from the file | ||
| // fallback into `out`. `allocator` is the allocator handed to the callback for the output buffer; pass nullptr to use |
There was a problem hiding this comment.
we should be explicit about the allocator ownership. e.g., this function will not take ownership of it, but the caller must be responsible for ensuring the OrtAllocator's lifetime spans beyond that of the EpContextData instance. it may be helpful to use one of the C++ API wrappers, like Ort::UnownedAllocator, to convey this.
| return ReadEpContextData(api, read_func, read_state, file_name, graph, out, allocator); | ||
| } | ||
|
|
||
| // std::vector<char> convenience overload that takes the read callback and its opaque state directly. Production EPs |
There was a problem hiding this comment.
with the EpContextData class, is the convenience provided by this std::vector overload still worth it? EpContextData provides a fairly convenient way to access the data already while avoiding copies.
| // reference. It is NOT a containment mechanism: it does not resolve symlinks and it rejects benign cases such as | ||
| // "a/b/c/../file.txt". Filesystem containment against a model directory is done by IsResolvedPathWithinBase(), which | ||
| // the untrusted (model-relative) resolution path uses. | ||
| inline bool ContainsPathTraversal(const std::filesystem::path& path) { |
There was a problem hiding this comment.
If these file_name values are genuinely callback-namespace names — opaque keys the application's I/O callback interprets — then I think the write-side path validation (ValidateEpContextDataName → ContainsPathTraversal / IsDirectoryOrEmptyName) should be dropped, not kept-and-clarified. Filesystem rules like "no ..", "not absolute", "not directory-like" have no meaning for an opaque namespace key, and applying them here can reject names an app legitimately chose for its own keyspace.
Supporting points:
- The write-callback branch already treats
file_nameas opaque and passes it through unmodified; only the no-callback file-fallback branch runsValidateEpContextDataName. So the validation is already inconsistent with the value's own namespace semantics — the fallback branch is the odd one out. - The value is only ever interpreted as a filesystem path on the read side, where
ResolveEpContextDataPath(graph != nullptr)+IsResolvedPathWithinBasealready do real model-directory containment (symlinks included). That's the correct place to enforce path safety — at the point of filesystem use — and dropping the write-side checks doesn't lose it: an unsafe stored reference is still rejected at read time.
Concretely: drop the ValidateEpContextDataName(file_name) call in the file-fallback write path (and with it the sole remaining use of ContainsPathTraversal, which then becomes dead code and can be removed). Note the physical write target is a separate parameter (fallback_file_name) that still goes through ResolveEpContextDataPath containment, so the on-disk write stays constrained — only the redundant logical-name check goes away.
Happy to be talked out of it if the intent was actually "this is a stored relative path, validate at authoring time" — but in that case it should be named/documented that way and applied consistently (callback branch included), not only in the fallback branch.
🤖 Written by GitHub Copilot on behalf of @edgchen1.
| // surface later as a confusing file I/O failure, so model-derived names like these are rejected up front. A non-leaf | ||
| // ".." in a logical callback-namespace name is rejected by ContainsPathTraversal(); in model-relative filesystem | ||
| // names it is canonicalized and accepted only if the resolved path stays within the model directory. | ||
| inline bool IsDirectoryOrEmptyName(const std::filesystem::path& path) { |
There was a problem hiding this comment.
IsDirectoryOrEmptyName is a slightly misleading name: it never stats the path — it's a lexical check on the leaf (filename() is empty, ., or ..). A real directory with a normal leaf name (e.g. a subdir/ that exists) passes this and only fails later at file open, so it doesn't actually detect "is a directory." As-is, its value is fail-fast error messaging (clearer than the eventual open-a-directory failure); the real gates are read-side containment + open().
If we want a check that actually means what the name says, a real std::filesystem::is_regular_file (via status(), which follows symlinks and is therefore consistent with weakly_canonical) on the read path would be more honest and would also reject special files. That last part closes a small real gap: on the untrusted read path, containment keeps us inside the model dir, but nothing stops a FIFO sitting there — opening a FIFO for read blocks, so it's a cheap DoS that neither this lexical check nor ifstream open catches.
Placement caveat: ResolveEpContextDataPath is shared by read and write. Read wants exists && is_regular_file; write usually targets a not-yet-existing file, so it can only require "if it exists, it must be regular." So the type check would live in the read/write callers (or the resolver gains a read/write mode), not as one unconditional call. It's also TOCTOU-advisory (best-effort, not a security boundary — a race-free version would fstat the open fd). If we add it, IsDirectoryOrEmptyName could then be dropped.
Note this pairs with my comment on ContainsPathTraversal/ValidateEpContextDataName: if that logical-name validation is dropped, IsDirectoryOrEmptyName's only remaining caller is this ResolveEpContextDataPath read/write-fallback resolution — which is exactly where the real is_regular_file check belongs. Non-blocking: the rename + comment fix is the minimum; the is_regular_file check is the nice-to-have.
🤖 Written by GitHub Copilot on behalf of @edgchen1.
Follow-up to #28624 (now merged) addressing review comments on the sample-only EPContext helper (
onnxruntime/test/autoep/library/ep_context_data_utils.h), plus a design doc for the overall feature. Targetsmain.Changes
1. Reject a leaf
..inIsDirectoryOrEmptyNameIsDirectoryOrEmptyName()previously treated only.and a trailing separator as directory-like. An untrusted, model-derived name with a leaf..(e.g.sub/..) would resolve to the parent/model directory and only fail later as a confusing file I/O error. It is now rejected up front. A non-leaf..anywhere in the path is still handled byContainsPathTraversal()/ the model-directory containment check.2. Avoid the
std::vector<char>copy on readThe app read-callback path previously copied the allocator-provided buffer into a
std::vector<char>. Added a move-onlyEpContextDataowning buffer plusReadEpContextData(...)overloads:The
std::vector<char>ReadEpContextDataWithFileFallback(...)overloads remain as convenience wrappers (file path reads directly into the caller's vector; the callback path delegates to the zero-copy reader and copies once). The example EP now uses the zero-copyReadEpContextDatapath.3. Drop the coarse path-traversal guard from trusted path resolution
Now that
IsResolvedPathWithinBase()does real model-directory containment for untrusted model-relative names, the lexicalContainsPathTraversal()guard is no longer applied on the trustedgraph == nullptrbranch ofResolveEpContextDataPath()— trusted callers already may pass absolute paths and own their paths, so there is no model directory to contain against.ContainsPathTraversal()is now used solely byValidateEpContextDataName(), which validates the logical callback-namespace name written into the modelep_cache_contextattribute (never resolved against a filesystem base). Untrusted model-relative names remain constrained byIsResolvedPathWithinBase().4. Adopt the callback buffer only on success
ReadEpContextData(...)now holds any callback-allocated buffer in a local RAII guard and transfers ownership into theEpContextDataout-parameter only after the callback status and(buffer, size)validation pass. On any error path the guard frees the buffer and the owner stays empty, matching the reset-first / bytes-on-success contract (and thestd::vectoroverload's empty-on-failure guarantee).5. Clear the
std::vector<char>output before validatingfile_nameThe
std::vector<char>ReadEpContextDataWithFileFallback(...)overload now clearsdataat the very top — before thefile_namenull/empty check — sodatais empty on every error path, consistent with theEpContextDataoverload which resetsoutbefore argument validation.6. Design doc:
docs/design/Compiled_Model_Encryption.mdAdds a design document for the application-controlled compiled-model encryption flow that #28624 and this PR implement: the named-buffer read/write callbacks (
OrtReadNamedBufferFunc/OrtWriteNamedBufferFunc), theOrtEpContextConfighandle and itsOrtEpApiaccessors, theOrt::Experimental::EpContextConfigC++ wrapper, and theep_context_data_utilsreference helper. It follows the decision-record layout ofdocs/design/Experimental_C_API.md(Requirements, Approaches Considered, Rejected Alternatives, application flows, memory analysis, API summary, open questions).Tests
EpContextDataUtils_ReadEpContextDataAdoptsCallbackBufferZeroCopy— covers the callback-adopt and file paths.EpContextDataUtils_ReadEpContextDataLeavesOutputEmptyOnCallbackError— a callback that allocates then returns an error leaves theEpContextDataempty (adopt-on-success).EpContextDataUtils_ResolvePathRejectsUnsafeNames— added asub/..rejection; updated the two trusted-branch (graph == nullptr)../escape.ctxassertions to accept the name and surface a normal file-open failure rather than a traversal rejection.EpContextDataUtils_CallbackFallbackUsesCallbacks— added an empty-callback-payload case;EpContextDataUtils_ReadCallbackRejectsNullBufferForNonEmptyPayload— asserts stale output is cleared on a callback failure.EpContextDataUtils_ResolvePathAndInvalidArguments— asserts a pre-filled vector is cleared whenfile_nameis empty...rejection (ValidateEpContextDataName) and the symlink/containment coverage are unchanged.onnxruntime_autoep_testbuilds clean;*EpContextDataUtils*= 8 passed + 1 skipped (symlink test, expected on Windows).The three changed C++ files are clang-format clean (the added design doc is Markdown).