EPContext sample helper follow-ups: reject .. leaf names and zero-copy read#29294
Open
GopalakrishnanN wants to merge 3 commits into
Open
EPContext sample helper follow-ups: reject .. leaf names and zero-copy read#29294GopalakrishnanN wants to merge 3 commits into
.. leaf names and zero-copy read#29294GopalakrishnanN wants to merge 3 commits into
Conversation
added 2 commits
June 26, 2026 17:48
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.
GopalakrishnanN
force-pushed
the
GopalakrishnanN/EpContextHelperUtilities-followups
branch
from
abe505c to
4369676
Compare
Contributor
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.
Comment on lines
457
to
469
| void* ep_context_data = nullptr; | ||
| size_t ep_context_data_size = 0; | ||
| OrtStatus* status = read_func(read_state, file_name, allocator, &ep_context_data, &ep_context_data_size); | ||
| auto buffer_deleter = [&api, allocator](void* buffer_to_free) { | ||
| if (buffer_to_free != nullptr) { | ||
| // Best-effort free during cleanup; release any returned status without throwing. | ||
| Ort::Status free_status{api.AllocatorFree(allocator, buffer_to_free)}; | ||
| static_cast<void>(free_status); | ||
| } | ||
| }; | ||
| std::unique_ptr<void, decltype(buffer_deleter)> ep_context_data_guard(ep_context_data, buffer_deleter); | ||
| // Adopt whatever the callback allocated so `out` frees it via the allocator on destruction, even on the error paths | ||
| // below. Ownership transfers without copying the (potentially large) buffer. | ||
| out.api_ = &api; | ||
| out.allocator_ = allocator; | ||
| out.buffer_ = ep_context_data; | ||
| out.buffer_size_ = ep_context_data_size; | ||
|
|
||
| if (status != nullptr) { | ||
| return status; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #28624 (now merged) addressing review comments on the sample-only EPContext helper (
onnxruntime/test/autoep/library/ep_context_data_utils.h). Targetsmain; the diff is the three changes below.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().Tests
EpContextDataUtils_ReadEpContextDataAdoptsCallbackBufferZeroCopycovering the callback-adopt and file paths.sub/..rejection assertion toEpContextDataUtils_ResolvePathRejectsUnsafeNames.graph == nullptr) assertions inEpContextDataUtils_ResolvePathRejectsUnsafeNames:../escape.ctxis now accepted by the resolver and surfaces a normal file-open failure rather than a traversal rejection. Logical-name..rejection (ValidateEpContextDataName) and the symlink/containment coverage are unchanged.onnxruntime_autoep_testbuilds clean;*EpContext*18 passed + 1 skipped (symlink test, expected on Windows).All three changed files are clang-format clean.