Skip to content

buffer source before reallocation in string::append(InputIt, InputIt)#1170

Open
Ramya-9353 wants to merge 2 commits into
boostorg:developfrom
Ramya-9353:string-append-iter-alias
Open

buffer source before reallocation in string::append(InputIt, InputIt)#1170
Ramya-9353 wants to merge 2 commits into
boostorg:developfrom
Ramya-9353:string-append-iter-alias

Conversation

@Ramya-9353

Copy link
Copy Markdown
Contributor

Repro: on a heap string with no spare capacity, self-append through the iterator overload, e.g. s.append(s.begin(), s.end()) or s.append(s.subview(k).begin(), s.subview(k).end()).
Cause: append(InputIt, InputIt) sends random-access iterators to a path that grows the buffer with string_impl::append and then std::copy(first, last, out). The append reallocates and frees the old buffer that [first, last) points into, so the copy reads freed memory (ASAN heap-use-after-free at string.hpp:206). The string_view and input-iterator overloads already avoid this; the random-access one copied straight from the aliased source.
Fix: when the append would reallocate, buffer the characters through the input-iterator path first, and keep the in-place copy when there is spare capacity. std::string defines this overload as append(basic_string(first, last)), so self-referential ranges are well defined.

Regression tests in test/string.cpp self-append through the iterator overload with capacity consumed so the append reallocates; they report a use-after-free before the change and pass after.

@cppalliance-bot

cppalliance-bot commented Jul 10, 2026

Copy link
Copy Markdown

An automated preview of the documentation is available at https://1170.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-07-17 10:24:25 UTC

@cppalliance-bot

cppalliance-bot commented Jul 10, 2026

Copy link
Copy Markdown

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

Build time: 2026-07-17 10:53:46 UTC

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1170   +/-   ##
========================================
  Coverage    93.91%   93.92%           
========================================
  Files           91       91           
  Lines         9265     9279   +14     
========================================
+ Hits          8701     8715   +14     
  Misses         564      564           
Files with missing lines Coverage Δ
include/boost/json/impl/string.hpp 100.00% <100.00%> (ø)

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 c57359d...b51e757. 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

@grisumbras

Copy link
Copy Markdown
Member

Thank you for catching this, but I am not sold on this implementation. Degrading to InputIterator implementation is quite a pessimization.

What really should happen is the library should

  1. Allocate new storage.
  2. Copy the new characters into the corrsponding positions.
  3. Copy old characters.
  4. Swap storages and destroy the old storage.

Something like

auto const n = static_cast<size_type>(last - first);
char const* p = data();
string_impl tmp;
if( n > capacity() - size() )
{
   tmp = string_impl(size() + n, sp_);
   p = temp.data();
}

std::copy(firt, last, p + n);

if( p != data() )
{
    std::memcpy( p, data(), size() );
    impl_.destroy(sp_);
    impl_ = tmp;
}

Also, I feel like of the two new tests the first one is unnecessary.

@Ramya-9353

Copy link
Copy Markdown
Contributor Author

Reworked as suggested: on reallocation the new characters are copied into the new storage while the old buffer is still live, then the old characters, then the storages are swapped and the old one destroyed. One deviation from the sketch: the new capacity comes from string_impl::growth(size + n, capacity()) rather than size() + n, so the growth factor stays consistent with string_impl::append and repeated appends remain amortised. Also dropped the first test.

The string suite passes ASAN-clean, as does an aliasing stress (whole and partial self-append, with and without spare capacity, checked against std::string semantics).

@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