fix: validate lower bound for dependency indices in upb file_def#26562
fix: validate lower bound for dependency indices in upb file_def#26562KevinZhao wants to merge 1 commit intoprotocolbuffers:mainfrom
Conversation
|
Thanks for the candidate change! Looks like the unit test fails for a different reason than the expected case, can you fix? |
|
Thanks @esrauchg! Fixed in the latest two commits: Commit 1 — Fixed the test: removed Commit 2 — While reviewing I also found a pre-existing bug: The "safe for tests" tag is needed for the fork CI to run the full test suite. |
esrauchg
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
| #include "upb/reflection/def.h" | ||
| #include "upb/reflection/def.hpp" | ||
|
|
||
| namespace upb_test { |
There was a problem hiding this comment.
Do you mind sending a separate PR with this fuzz test? I'd like another maintainer to review this part, but I'm comfortable approving the other two files of this PR
Thanks!
Adds a fuzz test that exercises upb DefPool file loading with arbitrary FileDescriptorSet protos. The test covers dependency index validation paths in file_def.c, including the public_dependency and weak_dependency accessors. Split from protocolbuffers#26562 per reviewer request. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
35f4843 to
dab13e5
Compare
|
@esrauchg Done — split the fuzz test into a separate PR: #26720 This PR now contains only the two files you're comfortable approving:
Squashed into a single clean commit for easy review. |
dab13e5 to
cfdb250
Compare
Adds a fuzz test that exercises upb DefPool file loading with arbitrary FileDescriptorSet protos. The test covers dependency index validation paths in file_def.c, including the public_dependency and weak_dependency accessors. Split from protocolbuffers#26562 per reviewer request. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cfdb250 to
5b66719
Compare
Adds a fuzz test that exercises upb DefPool file loading with arbitrary FileDescriptorSet protos. The test covers dependency index validation paths in file_def.c, including the public_dependency and weak_dependency accessors. Split from protocolbuffers#26562 per reviewer request. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add lower-bound (<0) validation for public_dependency and weak_dependency indices in _upb_FileDef_Create(), preventing negative indices from passing the bounds check and causing out-of-bounds reads on the deps array. - Fix upb_FileDef_WeakDependency() assert: was checking against public_dep_count instead of weak_dep_count. - Add unit tests for negative indices and zero-index-with-no-deps boundary cases.
5b66719 to
b66b950
Compare
Adds a fuzz test that exercises upb DefPool file loading with arbitrary FileDescriptorSet protos. The test covers dependency index validation paths in file_def.c, including the public_dependency and weak_dependency accessors. Split from protocolbuffers#26562 per reviewer request.
|
Hi @esrauchg — the fuzz test has been split into a separate PR (#26720) as requested. This PR now only contains the two files you mentioned being comfortable with:
CLA check is also passing now. Could you re-review when you get a chance? Thanks! |
Summary
< 0) validation forpublic_dependencyandweak_dependencyindices in_upb_FileDef_Create(upb/reflection/file_def.c)>= dep_count). Since both the index (int32_t) anddep_count(int) are signed, negative values like-1pass the comparison (-1 >= 3is false in signed arithmetic), get stored, and cause an out-of-bounds read when later used to index thedepsarray viaupb_FileDef_PublicDependency()orupb_FileDef_WeakDependency()public_dependencyandweak_dependencyvaluesfile_def_fuzz_test.cc) that exercises the dependency accessor code paths afterDefPool.AddFile(), covering index validation gaps that the existingdef_to_proto_fuzz_testdoes not reachTest plan
bazel test //upb/reflection:reflection_test— new tests verify negative indices are rejected with "out of range" errorbazel test //upb/reflection:def_builder_test— existing tests still passpublic_dependency: -1on a file with dependencies causes SIGSEGV inupb_FileDef_PublicDependency(); after the fix,_upb_FileDef_Createrejects it with an error