Repository navigation
Fix external data validation for HF shared blob caches - #33974
Open
Minsoo Kim (33modeling) wants to merge 3 commits into
Open
Minsoo Kim (33modeling) wants to merge 3 commits into
Minsoo Kim (33modeling) wants to merge 3 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A symlink-plus-.. path can cause both validators to grant shared-cache access to an outside model because lexical normalization differs from filesystem resolution.
5 open issues
What changed in this PR
This PR addresses #33950 by extending ONNX Runtime’s external-data validation to recognize Hugging Face shared blob caches, where models and weights can reside in different hash-prefix directories.
Changes:
- Adds shared-cache recognition to both pre-open and opened-file validators while retaining the legacy fallback.
- Checks cache markers, repository layout, blob names, and shard paths, including mixed local/shared storage.
- Adds cache-layout controls and an end-to-end inference test using generated external weights.
| File | Description |
|---|---|
onnxruntime/test/framework/tensorutils_test.cc |
Adds shared-cache validation cases and an external-initializer inference test. |
onnxruntime/core/framework/tensorprotoutils.cc |
Adds shared-cache recognition to both external-data path validators. |
Review effort: Balanced
|
|
||
| // Directory symlinks must not redirect the logical snapshot outside this repo. | ||
| const auto model_dir = repo / apparent_model.parent_path().lexically_relative(apparent_repo); | ||
| if (fs::canonical(apparent_model.parent_path(), ec) != model_dir || ec) return false; |
Comment on lines
+483
to
+486
| while (!snapshot.empty() && snapshot != snapshot.root_path() && | ||
| snapshot.parent_path().filename() != "snapshots") { | ||
| snapshot = snapshot.parent_path(); | ||
| } |
Comment on lines
+490
to
+491
| const auto hub = fs::canonical(apparent_repo.parent_path(), ec); | ||
| if (ec) return false; |
| EXPECT_FALSE(utils::ValidateExternalDataPath(snapshot_ / "model.onnx", "weights.bin").IsOK()); | ||
| } | ||
|
|
||
| #if !defined(ORT_MINIMAL_BUILD) && !defined(DISABLE_EXTERNAL_INITIALIZERS) |
Comment on lines
+1047
to
+1048
| const float expected = 42.0f; | ||
| data_file.write(reinterpret_cast<const char*>(&expected), sizeof(expected)); |
2 of 3 tasks
Tianlei Wu (tianleiwu)
added a commit
that referenced
this pull request
Nov 13, 2026
…34099) ### Description Since `huggingface_hub` 1.32, the Hugging Face Hub cache stores Xet files once per cache in a [blob store shared across repos](https://huggingface.co/docs/huggingface_hub/guides/manage-cache#shared-blobs-across-repos) (`<cache>/blobs/<hash prefix>/<hash>`), and the repo's `blobs/<etag>` becomes a relative symlink to it. A model and its external data in one snapshot can then resolve into different folders (for example `blobs/28/…` and `blobs/d4/…`, or one in the repo's `blobs/` and one in the shared store). The symlink fallback from #27374 only accepts external data next to the resolved model, so models with external data loaded from a default `hf_hub_download` path fail with `External data path escapes model directory`. This PR teaches both external data validators about the shared store without trusting anything outside the cache's blob folders. ### Summary of Changes #### Validation (`onnxruntime/core/framework/tensorprotoutils.cc`) New Pass 3 in `ValidateExternalDataPath`, reached only when the model is a symlink and Passes 1–2 fail. If the canonical model directory is in a snapshot of a cache repo (`<cache>/<type>s--<repo id>/snapshots/<revision>/…`, also from a subfolder such as `onnx/`), external data is accepted when: 1. its path does not leave the model directory before symlinks are resolved (no `..` escape), and 2. both the model and the external data resolve to files under the blob stores of that cache: the repo's `blobs/` folder or the cache-wide `blobs/` folder. `ValidateOpenedExternalDataPath` applies the same blob-store check to the canonical path of the opened file, so the opened-file validation from #33063 also covers the new case. The cost is one extra canonicalization, and only after the existing passes fail. | File | Change | |------|--------| | `onnxruntime/core/framework/tensorprotoutils.cc` | Add `IsInHuggingFaceCacheBlobStores` and use it in both validators. | | `onnxruntime/test/framework/tensorutils_test.cc` | `HuggingFaceCachePathValidationTest`: 11 tests on a cache built like `huggingface_hub` does, with two-hop relative symlinks (6 accepted layouts, 5 rejected). | | `onnxruntime/test/python/onnxruntime_test_python_symlink_data.py` | End-to-end `InferenceSession` test for the shared store and for a mixed repo-local/shared layout. | #### Security notes - Paths are compared in canonical form. A canonical path has no symlinks, so a file whose canonical path is under `<cache>/blobs` is stored in that real folder. If a `blobs/` folder is itself a symlink or junction, nothing matches (`huggingface_hub` also never uses a symlinked `blobs/` folder as the shared store). - The model itself must resolve into the blob stores. This rejects symlinked models that point outside the cache, and `symlink/..` tricks in the model path. - The no-`..` requirement keeps a malicious model from reaching other cached repos' files. In a real cache, the entries of a snapshot folder are symlinks that `huggingface_hub` creates to files of the same repo. - Someone who can write into the cache root gains nothing new. With the existing Pass 2 (for example ORT 1.31.1), they can already make a model read any blob by putting the model file in the same shard folder (verified). `huggingface_hub` also gives the store and shard folders the same permissions as the cache root. - Unchanged: empty, absolute, and in-memory-tag locations are still rejected before any fallback, and Passes 1–2 behave as before. #### Comparison with #33974 #33974 targets the same issue. It also checks the `.huggingface-shared-blobs` marker (version `1`) and blob names (hex, 40/64 characters, 2-character shard), and requires a `models--` repo folder. These checks are not security boundaries, since anyone who can create the layout can also create the marker and names. They do tie ORT to `huggingface_hub` internals, so a future layout change would break loading again. I built both and loaded the same synthetic caches: | Scenario | #33974 | This PR | |---|---|---| | Shared store, model and data in different shards | loads | loads | | Mixed repo-local/shared blobs (both directions) | loads | loads | | `datasets--…` repo | fails | loads | | Model folder reached through a symlinked directory | fails | loads | | Shared store marker version other than `1` | fails | loads | | Location `../x`; data symlink outside the cache; symlinked `blobs/` | rejected | rejected | ### Motivation and Context Fixes #33950. Downstream projects currently work around it by pinning `huggingface_hub<=1.31`, setting `HF_HUB_DISABLE_SHARED_BLOBS=1`, or copying files out of the cache. ### Testing - Real cache: reproduced the issue with `onnxruntime==1.31.1` and `huggingface_hub==2.4.0` (`onnx-community/sam2.1-hiera-tiny-ONNX`, fresh `HF_HOME`). With this PR, the same cache loads. The two mixed layouts created by downloading one of the files with `HF_HUB_DISABLE_SHARED_BLOBS=1` also fail with 1.31.1 and load with this PR. - `onnxruntime_test_all --gtest_filter='HuggingFaceCachePathValidationTest.*:PathValidationTest.*:GetExtDataFromTensorProtoTest.*'`: all pass. Without the `tensorprotoutils.cc` change, the 6 tests for accepted layouts fail. Removing any one of the conditions (no `..` escape, model in a blob store, `blobs/` folder bound, repo folder name, symlinked `blobs/`, opened-file check) makes the targeted test fail. - `python -m pytest onnxruntime/test/python/onnxruntime_test_python_symlink_data.py`: 4 passed (plus 2 subtests). - Full `onnxruntime_test_all` on Linux x64 (CPU, Release): 2897 passed. `lintrunner`: no issues. - Not run locally: Windows and WASM. The new tests skip when symlinks cannot be created, and the load test is excluded on WASM like the other external-file tests in that file. ### Checklist - [x] Tests added/updated - [x] No breaking changes - [ ] Documentation updated (not applicable) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
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.
HF Hub's shared blob store can place a cached ONNX model and its external weights in different SHA-256 prefix directories. The current symlink fallback confines weights to the resolved model's immediate parent, so these valid cache layouts fail to load.
Recognize the additional shared-cache layout in both the pre-open path validator and the opened-file validator. The allowance requires a model snapshot under a
models--...repository, a regular version-1.huggingface-shared-blobsmarker, non-redirected cache directories, and canonical blob targets with valid hash/shard paths. Also support mixed repository-local and shared blobs. Keep existing validation and the legacy same-directory fallback; logical directory escapes and absolute locations remain rejected.Tests add real two-hop relative symlinks and nested model paths, mixed cache layouts, marker/shard/escape/outside/missing-file controls, and a generated ONNX model whose external float initializer is loaded and executed through
InferenceSession.Validation on Linux x86-64, GCC 13, full CPU Debug build (
-O0 -g0, assertions and-Werrorretained):onnxruntime_test_all: 94 passed, including all 16 shared-cache cases, existing path/TensorProto security tests and external loader lifetime/task/manager tests.tools/ci_build/build.pytarget build andlintrunnerpassed. No downloaded model or external inference service was used.Related #33062 changes the model-path/file-handle plumbing; this patch targets the two current validators and may need integration adjustments if that refactor lands first.
Fixes #33950
Review follow-up: recognize the actual repository ancestor when nested model directories are named snapshots; compare the canonical original model directory and original model file; consistently use the Env-backed canonicalization for AppContainer fallback. Exclude native session tests on WASM and write weights with WriteLittleEndian. The nested-snapshots case failed before this follow-up; both symlink-parent outside-model controls were already rejected on Linux. No exploit reproduction is claimed. Windows/WASM/big-endian hosts were not run locally.