[io] Fix crash reading emulated collections of SSO strings by guitargeek · Pull Request #22925 · root-project/root

guitargeek · 2026-07-27T03:46:09Z

TEmulatedCollectionProxy stores the collection in a std::vector of raw aligned bytes. When that buffer grows past its capacity, Expand() lets it relocate the existing elements with a raw memory copy. That is correct for trivially relocatable content, but corrupts emulated objects that keep a pointer into themselves -- most notably a libstdc++ std::string (and TString) using the small-string optimization, whose internal data pointer keeps pointing into the old, now freed, buffer. The corrupted object then crashes when it is destroyed, freeing an invalid pointer.

TClass::Move(), invoked by Expand(), does not actually move the data for emulated classes, so it cannot fix up the relocated objects. Expand() is however only ever reached while preparing the collection to be entirely overwritten by a member-wise read, so when the buffer would reallocate and the value/key is a non-pointer class or string, destroy the (still valid) elements first and reconstruct all of them at the new location instead of relocating them.

This fixes the abort observed when reading e.g. a std::vector<std::pair<std::string,double>> without its dictionary.

Closes #20882

github-actions · 2026-07-27T05:39:06Z

Test Results

23 files 23 suites 3d 18h 33m 8s ⏱️
3 859 tests 3 859 ✅ 0 💤 0 ❌
79 509 runs 79 509 ✅ 0 💤 0 ❌

Results for commit 0c7ef47.

♻️ This comment has been updated with latest results.

dpiparo

Thanks for this PR. Before merging, maybe it's better to hear @pcanal 's opinion

dpiparo · 2026-09-02T07:30:48Z

This seems to have converged: @pcanal can it be merged?

TEmulatedCollectionProxy stores the collection in a std::vector of raw aligned bytes. When that buffer grows past its capacity, Expand() lets it relocate the existing elements with a raw memory copy. That is correct for trivially relocatable content, but corrupts emulated objects that keep a pointer into themselves -- most notably a libstdc++ std::string (and TString) using the small-string optimization, whose internal data pointer keeps pointing into the old, now freed, buffer. The corrupted object then crashes when it is destroyed, freeing an invalid pointer. TClass::Move(), invoked by Expand(), does not actually move the data, so it cannot fix up the relocated objects. Expand() is however only ever reached while preparing the collection to be entirely overwritten by a member-wise read, so when the buffer would reallocate and the value/key would not survive a raw memory copy, destroy the (still valid) elements first and reconstruct all of them at the new location instead of relocating them. Whether a raw memory copy is good enough is now decided by the new TClass::IsTriviallyRelocatable(), backed by a new kClassIsTriviallyRelocatable class property that TClingClassInfo::ClassProperty() fills in from Sema's implementation of C++26 trivial relocatability ([class.prop], P2786), via clang::Sema::IsCXXTriviallyRelocatableType(). Every trivially copyable class is trivially relocatable, but not vice versa: for example a polymorphic class whose bases and members are all trivially relocatable qualifies too. A positive answer rules out both freeing a resource twice or from the wrong address and skipping a non-trivial copy constructor. It is still not a proof -- `struct Foo { Foo *ptr = this; };` is trivially copyable, hence trivially relocatable, yet a raw memory copy leaves ptr pointing at the old location, and nothing observable here would reveal that -- but such a class is relocated exactly as it was before, so the check can never do worse than the unconditional memcpy it replaces. A class the interpreter does not know about, in particular an emulated one described only by a TStreamerInfo, gets the conservative answer, since its members can be anything, for example the std::string of an emulated std::pair<std::string,double>. TClass::Move() itself was silent about all this: it only records the address change in the object version repository and otherwise does nothing, leaving a caller that relocated the data with a raw memory copy no indication that the objects it just "moved" are now corrupted. It now reports an error, once per class, when asked to move a type that fails the same check. It cannot tell a raw memory copy from a real move, so the message is phrased conditionally; the in-tree callers avoid the memcpy for these types already, so this is aimed at external users of this public method. Note that Expand() now reconstructs the elements below nCurr instead of carrying their bytes over, so a member that is not written for a given entry is default-constructed rather than retaining the value from the previous one. That only shows up with schema evolution or unread sub-branches, and matches what the elements above nCurr have always done. A new test, roottest/root/io/emulatedGrow, writes a class holding a std::vector<std::pair<std::string,double>> whose collection grows on every entry and reads it back without its dictionary, so the emulated buffer has to reallocate repeatedly. It checks the values it reads back instead of comparing against a reference, so silent corruption fails it too. This fixes the abort observed when reading e.g. a std::vector<std::pair<std::string,double>> without its dictionary. Closes root-project#20882 🤖 Done with the help of AI.

The nolib test reads the STL containers back without the dictionary library, exercising the emulated collection proxy. It was flagged WILLFAIL because it crashed ROOT (and, before that, silently failed to compile) and its reference file dated back to the CINT/Makefile era, so it never matched. Now that the underlying crash is fixed (see previous commit), regenerate the reference from the current, deterministic output and drop the WILLFAIL flag, turning nolib into a real regression test that guards against the crash reappearing.

…::Expand When the storage of an emulated collection is reallocated, Expand() tells TClass::Move() where each element went so that the object version repository stays in sync. Two things were wrong with the addresses it passed. The value loop walked the elements from the start of the buffer, i.e. from the key, while the New() loop right below it correctly starts at fValOffset. For a map-like proxy the values therefore got registered under the addresses of their keys. This is harmless for a sequential container, where fValOffset is 0. Both loops also ran one iteration too many: the live elements are [0, nCurr), so the last iteration handed Move() an address one past the end of the old, about-to-be-freed buffer. Harmless in practice, since MoveAddressInRepository() only uses the addresses as map keys and never dereferences them. 🤖 Done with the help of AI.

The collection proxies derive byte offsets and buffer sizes from an element count and fValDiff, the distance between two consecutive elements. fValDiff is an int and the counts are UInt_t or int, so their product was computed in 32 bits and only widened afterwards, when it was assigned to a size_t or added to a pointer. The product therefore wraps well before the operands do: for the int * int cases in TGenCollectionStreamer it overflows at 2 GiB of payload, which a hundred million elements of thirty-odd bytes already reach. Note that `size_t len = fValDiff * nElements;` reads as if it were computed in 64 bits, but the multiplication has already truncated by then. Introduce TGenCollectionProxy::ElementOffset(), which multiplies in size_t, and use it wherever an element count is turned into a byte offset. The divisions by fValDiff are left alone: those widen correctly already. 🤖 Done with the help of AI.

guitargeek · 2026-09-03T17:14:13Z

Thank you very much for the review @pcanal! I have addressed all your comments.

pcanal

This looks good. Thanks.

guitargeek self-assigned this Jul 27, 2026

guitargeek requested review from bellenot, dpiparo, jblomer and pcanal as code owners July 27, 2026 03:46

guitargeek added in:Core Libraries AI-Assisted labels Jul 27, 2026

guitargeek force-pushed the issue-20882 branch from 6226f3a to 64b0fe2 Compare July 29, 2026 19:59

dpiparo approved these changes Aug 5, 2026

View reviewed changes

pcanal reviewed Aug 6, 2026

View reviewed changes

Comment thread io/io/src/TEmulatedCollectionProxy.cxx Outdated

pcanal reviewed Aug 6, 2026

View reviewed changes

Comment thread io/io/src/TEmulatedCollectionProxy.cxx Outdated

guitargeek force-pushed the issue-20882 branch 2 times, most recently from 464a868 to 0c7ef47 Compare August 9, 2026 16:18