Skip to content

document aliasing precondition for array::insert(pos, init) - #1174

Open
Ramya-9353 wants to merge 1 commit into
boostorg:developfrom
Ramya-9353:array-insert-initlist-uaf
Open

document aliasing precondition for array::insert(pos, init)#1174
Ramya-9353 wants to merge 1 commit into
boostorg:developfrom
Ramya-9353:array-insert-initlist-uaf

Conversation

@Ramya-9353

@Ramya-9353 Ramya-9353 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Repro: a.insert(a.begin(), {a[0], a[1]}) on an array with size() == capacity() reports an ASAN heap-use-after-free; the initializer_list<value_ref> overload builds revert_insert first, which on the growth path frees the old table before write_array dereferences the value_refs that point into it.
Resolution: per review, buffering the list into a temporary would defeat the point of the overload, so such use is declared UB instead. This adds the aliasing precondition to the docs, alongside the existing first/last precondition on the range overload. Callers needing the self-referential form can materialise a temporary themselves.

@cppalliance-bot

cppalliance-bot commented Jul 17, 2026

Copy link
Copy Markdown

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-14 11:49:06 UTC

@cppalliance-bot

cppalliance-bot commented Jul 17, 2026

Copy link
Copy Markdown

GCOVR code coverage report https://1174.json.prtest2.cppalliance.org/gcovr/index.html
LCOV code coverage report https://1174.json.prtest2.cppalliance.org/genhtml/index.html
Coverage Diff Report https://1174.json.prtest2.cppalliance.org/diff-report/index.html

Build time: 2026-08-14 12:01:11 UTC

@cppalliance-bot

Copy link
Copy Markdown

@Ramya-9353
Ramya-9353 force-pushed the array-insert-initlist-uaf branch from 4be67a7 to e8492c7 Compare August 12, 2026 09:34
@Ramya-9353

Copy link
Copy Markdown
Contributor Author

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.

@grisumbras

Copy link
Copy Markdown
Member

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

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.71%. Comparing base (bac1ab9) to head (58ee1d0).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             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     
Files with missing lines Coverage Δ
include/boost/json/array.hpp 100.00% <ø> (ø)

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bac1ab9...58ee1d0. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cppalliance-bot

Copy link
Copy Markdown

@Ramya-9353

Copy link
Copy Markdown
Contributor Author

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.

@grisumbras

Copy link
Copy Markdown
Member

Almost the entire point of this overload is to avoid an allocation of a temporary. Otherwise you could just do ja.insert(ja.end(), array{1, 2, 3}). Creating a temporary in its implementation begs the question if the overload should exist at all.

@Ramya-9353
Ramya-9353 force-pushed the array-insert-initlist-uaf branch from e8492c7 to 58ee1d0 Compare August 14, 2026 11:40
@Ramya-9353 Ramya-9353 changed the title buffer initializer_list before reallocation in array::insert document aliasing precondition for array::insert(pos, init) Aug 14, 2026
@Ramya-9353

Copy link
Copy Markdown
Contributor Author

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 first/last one on the range overload. Callers who do need the self-referential form can materialise the temporary themselves.

@cppalliance-bot

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants