Repository navigation
Fix VLA push_back/emplace_back of an aliased element during growth (#245) - #247
Conversation
) push_back(const T&), push_back(T&&) and emplace_back(Args&&...) grew the storage before reading their argument. When the argument referred to an element of the same array and the array was full, growth relocated that element and freed the old buffer first, so the new element was built from a moved-from object in released memory. All three now go through emplace_back, which on growth calls the new VariableLengthArrayBase::grow_and_emplace_back: - Without the allocator reallocate extension it allocates the new buffer, constructs the new element there while the old storage is intact, and then relocates the old elements, as libstdc++'s _M_realloc_insert does. A failure in either step destroys what was built and releases the new buffer, leaving the array unchanged. - For trivially copyable elements with a reallocating allocator, which may release the old block before anything is constructed, it builds the element in a temporary first and copies it into place after growth. Growth policy, allocation sequence, the length_error and bad_alloc behaviour, and the exceptions-disabled contract (size() unchanged when growth fails) are unchanged. VariableLengthArray<bool> takes its argument by value and is unaffected; a test confirms it. Tests check push_back and emplace_back of the first, a middle and the last element, by copy and by move, with and without reallocation, against std::vector, plus the reallocate path, allocation failure and construction failure rollback. Closes #245. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1aa64eb to
bb217f6
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb217f6a92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
On the reallocate path (trivially copyable elements, allocator with reallocate), grow_and_emplace_back built the new element in a stack temporary and then move-constructed it into place through the allocator. That needed a move constructor, so trivially copyable types with a deleted move constructor stopped compiling, and the allocator's construct hook ran twice for one append (plus a destroy for the temporary). The temporary is now relocated by copying its bytes, as reallocate() already does for the other elements. The allocator constructs the new element once and needs no move constructor, and it destroys the temporary only when the append fails. Tests add a copy-only trivially copyable element to the growth and allocation-failure tests, and an allocator that counts construct and destroy calls. Addresses Codex review comments on #247. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82f404fd01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
On the reallocate path, the appended element was constructed in a stack temporary and its bytes copied into the array afterwards. The allocator's construct hook therefore saw a stack address rather than the element, and a constructor that records `this` was left pointing at the expired temporary. The element is now always constructed once, through the allocator, at its final address. On the reallocate path the code depends on the arguments: - No arguments: grow as usual, then construct in place. - One argument of the element type (every push_back, emplace_back(a[i])): if it is not an element of this array, grow, then construct in place, with no temporary at all. If it is, first copy its bytes, which runs no constructor, then construct in place from the copy with the argument's value category. - Other constructor arguments may refer anywhere, so construct the element in new storage before relocating the old elements, as the non-reallocating path does. Only this case gives up reallocate. refers_to_element() moves from VariableLengthArray to its base so that insert() and append share it. Tests check that each append constructs exactly once at &back(), with and without aliasing, and that a failed append constructs nothing. Also that a constructor recording `this` sees its final address, and that emplace_back() without arguments still grows through reallocate. Addresses a Codex review comment on #247. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dceb304ec0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
On the reallocate path, an argument that is an element of the array was byte-copied into an inactive union member and the element constructed from that copy. Before C++20, copying bytes does not start an object's lifetime, so reading the copy was undefined behaviour in the C++14 and C++17 configurations. Such an argument now takes the construct-in-new-storage-then-relocate path instead, so this rare case forgoes reallocate and no copy is needed. The element-argument overload also matched volatile arguments, because std::decay drops volatile, and then failed to compile when binding them to a const reference. It now matches only T and const T arguments; volatile ones take the general path. Tests now record every address the allocator constructs at during an append and check that &back() appears exactly once, because on the relocating path the new element is constructed before the old ones. They also expect aliased appends not to call reallocate, and cover a volatile argument. Addresses Codex review comments on #247. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
push_back(const T&),push_back(T&&)andemplace_back(Args&&...)grew the storage before reading their argument. If the argument was an element of the same array and the array was full, growth relocated that element and freed the old buffer first. The new element was then built from a moved-from object in freed memory. For example,a.push_back(a[0])on a fullVariableLengthArray<std::string>gave an emptya[1].All three now go through
emplace_back. When it has to grow, it calls the newVariableLengthArrayBase::grow_and_emplace_back:_M_realloc_insertdoes. If either step throws, everything built so far is destroyed and the new buffer is released, so the array is unchanged.reallocatemay free the old block before anything is constructed. The element is still always constructed once, through the allocator, at its final address. The approach depends on the arguments:T/const Targument that is not an element of this array (the usualpush_back): grow throughreallocate, then construct in place.volatileones): use the usual path above, withoutreallocate.Some things stay the same:
length_errorandbad_allocbehaviour.size()is unchanged when growth fails.VariableLengthArray<bool>takes its argument by value, so it was not affected; a test confirms this. The now-unusedensure_size_plus_one()andpush_back_impl()helpers are removed from the primary template.Tests
test_variable_length_array_push_back.cppusesstd::vectoras the oracle. It covers:push_backandemplace_back, by copy and by move.std::stringandint.emplace_backwith constructor arguments that point into an element (a substring).boolspecialization across a storage-byte boundary.test_variable_length_array_reallocation.cpp:reallocateis used and when it is declined. This includes a copy-only trivially copyable type and avolatileargument.construct/destroycalls: each append constructs once, at the new element's address, and a failed append constructs nothing.thissees its final address.bad_alloc, or no effect with exceptions disabled.Before the fix, the new push_back suite crashes with a segfault and two of the new reallocation tests fail.
Validation
cetlvast/buildDebug and DebugEP.-fno-exceptions.-Wall -Wextra -Werror -Wconversion -Wsign-conversion -pedanticis clean:Closes #245.
🤖 Generated with Claude Code