[Core] Fix crash in get_id_for_file when hashing std::filesystem::path across ABI boundary#36729
[Core] Fix crash in get_id_for_file when hashing std::filesystem::path across ABI boundary#36729jatinwadhwa921 wants to merge 5 commits into
Conversation
|
build_jenkins |
There was a problem hiding this comment.
Pull request overview
Fixes a Linux/RHEL crash in Core tensor source tracking when std::filesystem::path objects created in a different binary/stdlib ABI are passed into OpenVINO and hashed for source-ID generation.
Changes:
- Replace
std::filesystem::hash_value(path)with hashing ofpath.native()inget_id_for_file()to avoid touching the ABI-sensitive “path components” representation. - Keep file ID generation as a combination of “path hash + (offset, size)” via
util::u64_hash_combine.
| inline uint64_t get_id_for_file(const std::filesystem::path& path, size_t offset, size_t size) { | ||
| return util::u64_hash_combine(std::filesystem::hash_value(path), {offset, size}); | ||
| return util::u64_hash_combine( | ||
| static_cast<uint64_t>(std::hash<std::filesystem::path::string_type>{}(path.native())), | ||
| {offset, size}); | ||
| } |
acf105f to
ac411a6
Compare
|
build_jenkins |
| TEST_F(OVTensorTest, getIdForFileMatchesIfstreamSourceId) { | ||
| // End-to-end: the ifstream read path (mmap=false) tags the tensor with get_id_for_file(...), | ||
| // computed over the file's byte size. Verify the exposed source_id equals a direct call, i.e. | ||
| // the value flows through read_tensor_data unchanged and without crashing. |
|
build_jenkins |
|
|
||
| inline uint64_t get_id_for_file(const std::filesystem::path& path, size_t offset, size_t size) { | ||
| return util::u64_hash_combine(std::filesystem::hash_value(path), {offset, size}); | ||
| return util::u64_hash_combine(static_cast<uint64_t>(std::hash<std::filesystem::path::string_type>{}(path.native())), |
There was a problem hiding this comment.
std::hash is also not ABI stable.
There was a problem hiding this comment.
This value is just a temporary key in an in-memory table that OV uses to reuse already-loaded weights during a single run. It's never saved to disk or shared between programs, so the number doesn't need to match across systems, it only needs to not crash and stay the same within one run.
The old code crashed because it hashed the path in a way that depended on the object's internal layout, which differed across the ABI boundary. The new code hashes the plain text of the path instead, which is the same everywhere , so it's safe.
I also tested it on the original crash case: it crashes before this change and works after.
There was a problem hiding this comment.
Right now fix replace one unstable solution with others. any of this std function are not stable.
I don't see reason why is changed the std::filesystem::hash_value is dedicated to create the hash and available in C++17.
Maybe used tools are to old to as they not correctly support C++17?
Or not compatible library is used.
From OV perspective it looks like not needed change as std functions works
There was a problem hiding this comment.
Thanks for the feedback, you're right that swapping one std hash for another didn't address the underlying concern, so I've reworked it. The latest change removes all std-library hashing: it folds the raw bytes of path.c_str() through OpenVINO's own u64_hash_combine, so the id no longer depends on std::filesystem::hash_value, std::hash, or any out-of-line std symbol.
|
|
||
| // Regression tests for ov::util::get_id_for_file: hashing the path must not cross a libstdc++ | ||
| // ABI boundary (the crash it used to trigger) and must yield a stable, distinct id per file. | ||
| TEST_F(OVTensorTest, getIdForFileDoesNotCrashAndIsNonZero) { |
There was a problem hiding this comment.
Will any of this test fail without the changes?
There was a problem hiding this comment.
No, they won't fail without the change. The crash only happens when two modules are built with different ABI settings, and the unit-test binary is a single unified build, so it can't reproduce that condition. The actual crash fix was verified separately on the original reproducer (crashes before, works after).
There was a problem hiding this comment.
If they not fail there is no value of such test. Can be added something can valid it.
There was a problem hiding this comment.
A test that fails without the fix would need to reproduce the crash, but the crash only occurs under an ABI mismatch between two modules. The unit-test binary is a single unified build, so it can't create that condition. I will remove this contract test but i don't think so that any test could be added which can check ABI mismatch.
Problem
get_id_for_file() creates a unique id for a file by hashing its path. It did this with std::filesystem::hash_value(path).
The catch: a std::filesystem::path stores the path in two ways — a plain string (the full path on one line) and a list of separate pieces (each folder/file name split out). hash_value() reads the list of pieces. Where that list sits in memory is not guaranteed to be the same across binaries built with different C++ standard libraries.
So when a path is created in one module and passed into read_tensor_data() inside an OpenVINO binary that was built differently, OpenVINO looks for the "list of pieces" in the wrong spot, reads garbage memory, and crashes in std::_Hash_bytes.
Where it hit
ONNX Runtime's OpenVINO EP reloads a compiled blob by calling ov::read_tensor_data(path). On RHEL, the ORT binary and the OpenVINO binary are built with different C++ standard-library (ABI) settings, so they disagree on how a path is laid out in memory:
The plain-string part lines up in both → opening/mmapping the file works fine.
The list-of-pieces part is in a different place → hashing it crashes.
This started in 2026.2, which added the id/source-tracking feature that hashes the path here; 2026.1 didn't hash at all, so it never crashed. Windows isn't affected (it builds the id a different way, via GetFileInformationByHandle), and Ubuntu happens to build both binaries with matching ABIs, so it works there too.
Ticket id -> CVS-190300