Skip to content

Commit 0d74f31

Browse files
gitamohrpixar-oss
authored andcommitted
usd: Fix a bug where corrupt .usdc files could cause a data race opening a
file. (Internal change: 2363810)
1 parent 86f87b3 commit 0d74f31

File tree

4 files changed

+100
-7
lines changed

4 files changed

+100
-7
lines changed

pxr/usd/usd/CMakeLists.txt

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,17 @@ pxr_build_test(testUsdUsdzBugGHSA01
504504
testenv/testUsdUsdzBugGHSA01.cpp
505505
)
506506

507+
pxr_build_test(testUsdUsdcBugGHSA02
508+
LIBRARIES
509+
ar
510+
arch
511+
tf
512+
sdf
513+
usd
514+
CPPFILES
515+
testenv/testUsdUsdcBugGHSA02.cpp
516+
)
517+
507518
pxr_build_test(testUsdSplinesCpp
508519
LIBRARIES
509520
tf
@@ -795,6 +806,11 @@ pxr_install_test_dir(
795806
DEST testUsdUsdzBugGHSA01
796807
)
797808

809+
pxr_install_test_dir(
810+
SRC testenv/testUsdUsdcBugGHSA02.testenv
811+
DEST testUsdUsdcBugGHSA02
812+
)
813+
798814
pxr_register_test(testUsdAppliedAPISchemas
799815
PYTHON
800816
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdAppliedAPISchemas"
@@ -1417,3 +1433,9 @@ pxr_register_test(testUsdUsdzBugGHSA01
14171433
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdUsdzBugGHSA01"
14181434
EXPECTED_RETURN_CODE 0
14191435
)
1436+
1437+
pxr_register_test(testUsdUsdcBugGHSA02
1438+
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdUsdcBugGHSA02"
1439+
EXPECTED_RETURN_CODE 0
1440+
)
1441+

pxr/usd/usd/crateFile.cpp

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3702,22 +3702,36 @@ CrateFile::_ReadCompressedPaths(Reader reader,
37023702
cr.Read(reader, pathIndexes.data(), numPaths);
37033703

37043704
#ifdef PXR_PREFER_SAFETY_OVER_SPEED
3705-
// Range check the pathIndexes, which index into _paths.
3706-
for (const uint32_t pathIndex: pathIndexes) {
3707-
if (pathIndex >= _paths.size()) {
3708-
TF_RUNTIME_ERROR("Corrupt path index in crate file (%u >= %zu)",
3709-
pathIndex, _paths.size());
3710-
return;
3705+
// Range check the pathIndexes, which index into _paths, and also ensure
3706+
// there are no duplicates in pathIndexes. If there are this file is
3707+
// corrupt, and if we were to continue we could data-race while mutating the
3708+
// same SdfPath object in _paths concurrently in
3709+
// _BuildDecompressedPathsImpl. It's a delightful occasion to use C++'s
3710+
// collectio non grata: vector<bool>.
3711+
{
3712+
vector<bool> seenIndexes(_paths.size());
3713+
for (const uint32_t pathIndex: pathIndexes) {
3714+
if (pathIndex >= _paths.size() || seenIndexes[pathIndex]) {
3715+
TF_RUNTIME_ERROR(
3716+
"Corrupt path index in crate file (%u %s)",
3717+
pathIndex,
3718+
pathIndex >= _paths.size()
3719+
? TfStringPrintf(">= %zu", _paths.size()).c_str()
3720+
: "repeated");
3721+
return;
3722+
}
3723+
seenIndexes[pathIndex] = true;
37113724
}
37123725
}
3726+
37133727
#endif // PXR_PREFER_SAFETY_OVER_SPEED
37143728

37153729
// elementTokenIndexes.
37163730
elementTokenIndexes.resize(numPaths);
37173731
cr.Read(reader, elementTokenIndexes.data(), numPaths);
37183732

37193733
#ifdef PXR_PREFER_SAFETY_OVER_SPEED
3720-
// Range check the pathIndexes, which index (by absolute value) into
3734+
// Range check the elementTokenIndexes, which index (by absolute value) into
37213735
// _tokens.
37223736
for (const int32_t elementTokenIndex: elementTokenIndexes) {
37233737
if (static_cast<size_t>(
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
//
2+
// Copyright 2025 Pixar
3+
//
4+
// Licensed under the terms set forth in the LICENSE.txt file available at
5+
// https://openusd.org/license.
6+
//
7+
8+
#include "pxr/pxr.h"
9+
10+
#include "pxr/base/tf/error.h"
11+
#include "pxr/base/tf/errorMark.h"
12+
#include "pxr/base/tf/diagnosticLite.h"
13+
#include "pxr/usd/usd/stage.h"
14+
15+
#include <string>
16+
#include <cstring>
17+
18+
PXR_NAMESPACE_USING_DIRECTIVE
19+
20+
// This test checks for the security issue detailed in Github security
21+
// advisory GHSA-58p5-r2f6-g2cj.
22+
static void
23+
TestUsdcFile()
24+
{
25+
// This test relies on range checks that are only enabled when
26+
// PXR_PREFER_SAFETY_OVER_SPEED is enabled.
27+
#ifdef PXR_PREFER_SAFETY_OVER_SPEED
28+
29+
TfErrorMark m;
30+
auto stage = UsdStage::Open("root.usdc");
31+
32+
// a runtime error should have been posted
33+
TF_AXIOM(!m.IsClean());
34+
35+
// Look for the specific runtime error for the invalid spec type
36+
auto decompressError = [](const TfError& e) -> bool {
37+
return TfStringEndsWith(e.GetCommentary(),
38+
"Failed to decompress data, "
39+
"possibly corrupt? LZ4 error code: -596");
40+
};
41+
TF_AXIOM(std::any_of(m.begin(), m.end(), decompressError));
42+
43+
// Make sure that a corrupt asset error was also posted
44+
auto corruptPathIndex = [](const TfError& e) -> bool {
45+
return e.GetCommentary() == "Corrupt path index in crate file "
46+
"(0 repeated)";
47+
};
48+
TF_AXIOM(std::any_of(m.begin(), m.end(), corruptPathIndex));
49+
#endif // PXR_PREFER_SAFETY_OVER_SPEED
50+
}
51+
52+
int main(int argc, char** argv)
53+
{
54+
TestUsdcFile();
55+
56+
return 0;
57+
}
Binary file not shown.

0 commit comments

Comments
 (0)