Fix insert(pos, count, value) overflowing instead of throwing - #76
Merged
Conversation
make_uninitialized_space() decided whether the elements still fit with "s + count > capacity()". A huge count wrapped that sum around to something small, the check said it fits, and the in place shift wrote past the end of the buffer. std::vector rejects the same call. Both comparisons are now subtractions, which cannot wrap because s is never larger than capacity() or max_size(), so each one is correct on its own rather than relying on a guard above it. The existing overflow checks did not help: calculate_new_capacity() only ever sees an already summed size, and storage<T>::alloc() guards capacity * sizeof(T) instead. Both sit behind the wrapped comparison that decided not to reallocate at all. Throws std::bad_alloc, which is what this library uses everywhere for a request that is too large. std::vector would throw std::length_error here, but calculate_new_capacity() and reserve() already made the other choice and being inconsistent within svector seems worse than the existing difference. Closes #69 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #69.
s + countwrapped, the wrapped sum looked small enough to fit, so the in-place shift ran past the end of the buffer.Fix
Both comparisons are now subtractions:
Neither can wrap, since
sis never larger thancapacity()ormax_size(). The review pointed out that leaving the second one ass + count > capacity()and relying on the guard above would be a cross-line invariant a later edit could quietly break — each check is now correct on its own.The existing overflow checks could not have caught this:
calculate_new_capacity()only ever receives an already-summed size, andstorage<T>::alloc()guardscapacity * sizeof(T). Both sit behind the wrapped comparison that decided not to reallocate at all.Audit of the other arithmetic
Checked every other size computation in the header.
reserve,resize,resize(count, value),svector(count),svector(count, value),assign(count, value)andresize_and_overwriteall pass a single bounded value intocalculate_new_capacity, which already checks it againstmax_size()— no sum of two unbounded values, so nothing to wrap.emplace_back_grow'ss + 1is bounded by the size invariant. Everys + countinmake_uninitialized_space_newis now downstream of the new guard. This was the only one.Exception type
std::bad_alloc, matchingcalculate_new_capacity()andreserve().std::vectorthrowsstd::length_errorfor this specific case, which is arguably the better contract for a drop-in replacement, buttest/unit/bad_alloc.cppalready pinsreserve(max_size())tobad_alloc— introducinglength_errorfor this one check would be a worse inconsistency than the existing divergence. Changing it library-wide is a separate decision.Cost
Reviewed the codegen:
max_size()folds to an immediate, and the check ismovabs/sub/cmp+jb. Benchmarked single-element insert, emplace, and count inserts — all within noise.Tests live in
test/unit/insert.cppnext to the other insert coverage; reverting the guard reproduces the stack-buffer-overflow.