[Bug libstdc++/127228] New: basic_string::_M_create not honoring contracts

[samuelgardner101 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=127228

            Bug ID: 127228
           Summary: basic_string::_M_create not honoring contracts
           Product: gcc
           Version: 17.0
            Status: UNCONFIRMED
          Keywords: ABI, wrong-code
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: samuelgardner101 at gmail dot com
  Target Milestone: ---

`basic_string::_M_create` no longer honours its old contract: it writes a grown
capacity back through its reference parameter when `__old_capacity` is 0. gcc
pre-17 inlined callers use that same variable as the string's *length*, so a
`std::string` built by already-compiled code reports a `size()` larger than the
string, with the excess reading uninitialised heap. So a 25-character string
might report `size() == 31`, instead of `size() == 25` (see example beneath)

`_M_create`'s contract, as relied on by every pre-17 inlined caller, is that
with `__old_capacity == 0` it leaves `__capacity` alone: its only growth rule
is `__capacity > __old_capacity && __capacity < 2 * __old_capacity`, which
cannot fire when `__old_capacity` is `0`.


EXAMPLE

```c++
// repro.cc
#include <string>
#include <cstdio>

#ifdef OLD_HEADERS
// Compiled against libstdc++ <= 16. Its inline _M_construct is baked in here,
and that code does
//     _M_data(_M_create(__dnew, 0));  _M_capacity(__dnew);  ... 
_M_set_length(__dnew);
// - one variable for the request, the granted capacity and the length.
void check() {
  const char *p = "0123456789012345678901234";           // 25 characters
  std::string s(p, p + 25);
  std::printf("size()=%zu (expected 25)  capacity()=%zu\n", s.size(),
s.capacity());
}
#else
// Compiled against libstdc++ 17 at C++23. Using std::string is enough to emit
this translation unit's
// own copy of basic_string<char>::_M_create_plus, and at C++23 that copy
rounds the capacity up via
// allocate_at_least.
void check();
static std::string join(const char *a, const char *b) { return std::string(a) +
b; }
int main() {
  const std::string s = join("some text that is long enough", " to be heap
allocated");
  std::printf("(unrelated string of %zu chars)\n", s.size());
  check();
}
#endif
```

```sh
$ g++ -std=c++17 -O2 -DOLD_HEADERS -nostdinc++ \
      -isystem /usr/include/c++/14 -isystem
/usr/include/x86_64-linux-gnu/c++/14 \
      -c repro.cc -o old.o
$ g++ -std=c++23 -O0 -c repro.cc -o new.o
$ g++ old.o new.o -o repro && ./repro
(unrelated string of 50 chars)
size()=31 (expected 25)  capacity()=31
```

so we compile repro.cc in the old build to create old.o, then compile the
repro.cc again as the new.o, with `check` stubbed to use the old.o version of
it. and we see the error. the last 6 characters are heap garbage.

```
second object at c++17 -O0   ->  _M_create(23, 0) writes back 23   (no rounding
path)
second object at c++20 -O0   ->  _M_create(23, 0) writes back 23
second object at c++23 -O0   ->  _M_create(23, 0) writes back 31   <-- GREW
second object at c++26 -O0   ->  _M_create(23, 0) writes back 31   <-- GREW
```


before `acfdad706d8`, `basic_string.tcc` had:

```c++
_M_create(size_type& __capacity, size_type __old_capacity)
{
  if (__capacity > max_size()) std::__throw_length_error(...);
  if (__capacity > __old_capacity && __capacity < 2 * __old_capacity)
    { __capacity = 2 * __old_capacity; ... }
  return _S_allocate(_M_get_allocator(), __capacity + 1);
}
```

and its caller, still present in every binary compiled against those headers:

```c++
size_type __dnew = static_cast<size_type>(std::distance(__beg, __end));
if (__dnew > size_type(_S_local_capacity))
  { _M_data(_M_create(__dnew, size_type(0))); _M_capacity(__dnew); }
...
_M_set_length(__dnew);
```

one variable carries the request, the granted capacity and the length, which is
sound only because `_M_create` cannot change it when `__old_capacity == 0`.
`acfdad706d8` renamed that function to `_M_create_plus`, changed it to allocate
with `_S_allocate_at_least`, and added a new `_M_create` in its place:

```c++
// This must remain for ABI stability, though unused in current code.
_M_create(size_type& __capacity, size_type __old_capacity)
{
  _Alloc_result __r = _M_create_plus(__capacity, __old_capacity);
  __capacity = __r.__count - 1;  // Leave room for NUL.
  return __r.__ptr;
}
```

`__capacity` now comes back as the `allocate_at_least` count regardless of
`__old_capacity`, and old callers store it as the length. Keeping the symbol
while changing what it does does not preserve the ABI.

---

suggested fix from Claude, no idea on viability:
```c++
_M_create(size_type& __capacity, size_type __old_capacity)
{
  if (__capacity > max_size())
    std::__throw_length_error(__N("basic_string::_M_create"));
  if (__capacity > __old_capacity && __capacity < 2 * __old_capacity)
    {
      __capacity = 2 * __old_capacity;
      if (__capacity > max_size()) __capacity = max_size();
    }
  return _S_allocate(_M_get_allocator(), __capacity + 1);
}
```

which is what the function did before acfdad706d8 - old callers then see
exactly what they saw when they were compiled; current code does not call
_M_create at all.