Fix signed integer overflow in RepeatedField MergeFrom variants#28190
Open
momo-123-coder wants to merge 1 commit into
Open
Fix signed integer overflow in RepeatedField MergeFrom variants#28190momo-123-coder wants to merge 1 commit into
momo-123-coder wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
momo-123-coder
force-pushed
the
fix/merge-from-internal-integer-overflow
branch
from
468e63e to
059da13
Compare
Author
|
/gclabot recheck |
1 similar comment
Author
|
/gclabot recheck |
MergeFromInternal<T> (line 176) and MergeFrom<MessageLite> (line 269) in repeated_ptr_field.cc computed the post-merge size with raw signed integer addition, which is undefined behavior when current_size_ + from.current_size_ exceeds INT_MAX. A negative new_size bypasses the capacity check in InternalReserve(), causing it to return a pointer past the end of the allocated buffer without growing it. The subsequent copy loop then writes from.current_size_ elements out of bounds, resulting in a heap-buffer-overflow. RepeatedField<Element>::MergeFrom in repeated_field.h had the same pattern in its Reserve() call. Apply internal::CheckedAdd() consistently to all three sites, matching the pattern already used in MergeFromConcreteMessage() (line 238).
momo-123-coder
force-pushed
the
fix/merge-from-internal-integer-overflow
branch
from
059da13 to
be0c4fa
Compare
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
1 participant
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.
Summary
MergeFromInternal<T>(repeated_ptr_field.cc:176) andMergeFrom<MessageLite>(repeated_ptr_field.cc:269) computed the post-merge element count using raw signedintaddition with no overflow protection.RepeatedField<Element>::MergeFrom(repeated_field.h:1261) passed a rawold_size + other_sizeexpression directly toReserve().current_size_ + from.current_size_ > INT_MAX, the addition wraps to a negative value (signed integer overflow — undefined behavior in C++). A negativenew_sizealways satisfies then <= Capacity()check inInternalReserve(), causing it to return a pointer past the end of the allocated buffer without growing it. The subsequent copy loop then writesfrom.current_size_elements out of bounds — a heap-buffer-overflow.MergeFromConcreteMessage()at line 238 of the same file already usesinternal::CheckedAdd()correctly. This PR applies the same pattern to the three missing sites.Changes
repeated_ptr_field.cccurrent_size_ + from.current_size_internal::CheckedAdd(current_size_, from.current_size_)repeated_ptr_field.cccurrent_size_ + from.current_size_internal::CheckedAdd(current_size_, from.current_size_)repeated_field.hReserve(old_size + other_size)Reserve(internal::CheckedAdd(old_size, other_size))Crash reproduction (AddressSanitizer + UBSan)
Test plan
RepeatedField<int32>whose combined size exceedsINT_MAX— verifyCheckedAddaborts cleanly instead of writing out of bounds