buffer initializer_list before reallocation in array::insert - #1174
buffer initializer_list before reallocation in array::insert#1174Ramya-9353 wants to merge 1 commit into
Conversation
|
An automated preview of the documentation is available at https://1174.json.prtest2.cppalliance.org/libs/json/doc/html/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-08-27 09:45:04 UTC |
|
GCOVR code coverage report https://1174.json.prtest2.cppalliance.org/gcovr/index.html Build time: 2026-08-27 09:49:26 UTC |
|
|
4be67a7 to
e8492c7
Compare
|
Rebased onto current develop to re-run CI. The only red stage on the previous drone build (Linux GCC 12 arm64) hung for ~44h and was killed by the runner; all other 65 stages passed, including the ASan/UBSan/TSan/Valgrind jobs. No code changes. |
|
I thought, I commented on this PR, but it appears that I have forgotten to do that. While I understand the problem, in this case the solution might be to just declare such use UB and hence motivate the caller to create the temporary themselves. Did you catch the bug in an organic usage scenario, or have you encountered it simply by analysing corner cases? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1174 +/- ##
===========================================
- Coverage 93.91% 93.71% -0.21%
===========================================
Files 91 85 -6
Lines 9288 8971 -317
===========================================
- Hits 8723 8407 -316
+ Misses 565 564 -1
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
|
|
Corner-case analysis, not an organic report. After the fill-value aliasing fix in #1167 I went through the remaining array mutators looking for reads of user-supplied sources after reallocation, and this overload was the last one left. On declaring it UB: that's defensible, but it would make this overload the odd one out. The count-fill and input-iterator overloads already tolerate aliasing, and the equivalent braced-list insert on std::vector is well defined because the list materialises copies before the call, so the pointer semantics of value_ref are what make it surprising here; nothing at the call site hints that a temporary is needed. The cost is one temporary array on this overload only, the same as the range insert already pays. That said, if you'd rather document it as a precondition instead, I can rework the PR that way. |
|
Almost the entire point of this overload is to avoid an allocation of a temporary. Otherwise you could just do |
e8492c7 to
58ee1d0
Compare
Ramya-9353
changed the title
|
Fair enough, the temporary does undercut the reason the overload exists. Reworked as you suggested: the code change and test are gone, and the doc block now declares the aliasing UB with a precondition next to the existing |
|
|
|
I was persuaded that my cost-benenfit analysis was wrong. Can you please return the PR to fix UB instead of documenting it? |
The initializer_list<value_ref> overload constructed revert_insert
before reading the list. On the growth path revert_insert frees the
old table, so value_refs pointing into the array itself were read
after the storage was freed: a heap use-after-free, e.g.
a.insert(a.begin(), {a[0], a[1]}) when size() == capacity().
Materialise the list into a temporary array before relocating,
matching the input-iterator insert path.
58ee1d0 to
3f2864c
Compare
Ramya-9353
changed the title
|
Done. Dropped the doc-only commit and restored the code fix: the init-list is buffered into a temporary array before revert_insert runs, same shape as the input-iterator overload, with an early return for the empty list. The regression test (self-referential init-list insert into a full array) is back too; it reports a heap-use-after-free under ASan on develop and the full array suite passes clean with the fix, ASan/UBSan. |
|
|
3 participants
Repro:
a.insert(a.begin(), {a[0], a[1]})on an array withsize() == capacity()reports an ASAN heap-use-after-free; theinitializer_list<value_ref>overload buildsrevert_insertfirst, which on the growth path frees the old table beforewrite_arraydereferences thevalue_refs that point into it.Fix: materialise the init-list into a temporary array before constructing
revert_insert, then relocate, matching the input-iterator insert path. The regression test inserts a self-referential init-list into a full array; it reports a heap-use-after-free under ASAN before the fix and passes after.