Fix insert() of an empty range blanking the tail - #71
Merged
Conversation
shift_right(p, data + s, p + count) with count == 0 has its target begin equal
to its source begin, so std::move_backward was called with its destination
equal to its source end. That is a precondition violation, and what it actually
does is self-move-assign every element from pos to the end. With std::string
they all come back empty.
Reachable from insert(pos, 0, value), insert(pos, first, first) with forward
iterators, and insert(pos, {}). The input iterator overload already returns
early for an empty range, which is why only the other two were affected.
Guarded in shift_right rather than at the call site: its documented
precondition is source_begin <= target_begin, non strict, so the equal case
was always supposed to work. Benchmarked both placements on single element
insert, no measurable difference.
The tests use std::string because the existing empty range coverage in this
file uses Counter::Obj, whose move assignment copies its fields, so a self move
preserves the value and the corruption is invisible. See #67.
Closes #65
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 #65.
v.insert(pos, 0, value)blanked every element fromposonwards. No exception, no sanitizer report, just wrong data:Cause
make_uninitialized_spacecallsshift_right(p, data + s, p + count). Withcount == 0the target begin equals the source begin, so the second step becomesstd::move_backward(first, last, last)— destination equal to source end, which that algorithm explicitly disallows. It walks backwards self-move-assigning every element in the range.Affects
insert(pos, 0, value),insert(pos, first, first)(forward iterators) andinsert(pos, {}). The input-iterator overload already had an empty-range early-out, which is why it escaped.Where the guard goes
In
shift_right, not at the call site. Its documented precondition issource_begin <= target_begin— non-strict, so the equal case was always advertised as valid input and the implementation simply did not honour it. Guarding the one caller instead would leave the primitive a footgun for the next one.The review raised that
count == 0at the call site is easier for the compiler to fold than pointer equality inside the callee. Benchmarked both placements:shift_rightinsert(begin(), value)intinsert(mid, value)intinsert(begin(), string)No difference, so correctness of the primitive wins.
Tests
Two cases in
test/unit/insert.cpp, covering all three affected overloads at every position including thepos == sizeboundary, and once more in indirect mode. Both fail without the guard.They use
std::stringdeliberately. This file already had empty-range insert coverage, but withCounter::Obj, whose move assignment is a plain field copy — a self-move preserves the value, so the corruption was unobservable. That blind spot is #67.Verified with gcc and clang under
-fsanitize=address,undefined.