[io] Fix crash reading emulated collections of SSO strings

README/ReleaseNotes/v642/index.md

@@ -93,8 +93,12 @@ Replace `obj.Connect(signal, "TPyDispatcher", disp, "Dispatch()")` with
 
 ## I/O
 
+* Reading a collection without its dictionary no longer crashes when the elements hold a `std::string` or a `TString`. The emulated collection proxy relocated its elements with a raw memory copy when its buffer had to grow, which corrupts an object that points into itself, such as a `std::string` using the small string optimization; the invalid pointer was then freed when the object was destroyed. Such elements are now destroyed and reconstructed at the new location instead. This affected for instance a `std::vector<std::pair<std::string,double>>` read back without a dictionary.
+
 ## Core
 
+* `TClass::IsTriviallyRelocatable()` reports whether an object of a class can be moved to a new address with a raw memory copy, i.e. without running a move or copy constructor (trivial relocatability in the C++26 sense). It is backed by the new `kClassIsTriviallyRelocatable` class property, which `TInterpreter::ClassInfo_ClassProperty()` now fills in. A class the interpreter does not know about, in particular an emulated one, is conservatively reported as not relocatable.
+
 ## Histograms
 
 ### Cumulative histograms in more than one dimension

core/meta/inc/TClass.h

@@ -530,6 +530,7 @@ friend class TStreamerInfo;
    Bool_t             InheritsFrom(const char *cl) const override;
    Bool_t             InheritsFrom(const TClass *cl) const override;
    void               InterpretedShowMembers(void* obj, TMemberInspector &insp, Bool_t isTransient);
+   Bool_t             IsTriviallyRelocatable() const;
    Bool_t             IsFolder() const override { return kTRUE; }
    Bool_t             IsLoaded() const;
    Bool_t             IsForeign() const;

core/meta/inc/TDictionary.h

@@ -132,20 +132,23 @@ enum EFunctionProperty {
    kIsTemplateSpec= 0x00000020
 };
 
+// clang-format off
 enum EClassProperty {
-   kClassIsValid         = 0x00000001,
-   kClassHasExplicitCtor = 0x00000010,
-   kClassHasImplicitCtor = 0x00000020,
-   kClassHasCtor         = 0x00000030,
-   kClassHasDefaultCtor  = 0x00000040,
-   kClassHasAssignOpr    = 0x00000080,
-   kClassHasExplicitDtor = 0x00000100,
-   kClassHasImplicitDtor = 0x00000200,
-   kClassHasDtor         = 0x00000300,
-   kClassHasVirtual      = 0x00001000,
-   kClassIsAbstract      = 0x00002000,
-   kClassIsAggregate     = 0x00004000
+   kClassIsValid                = 0x00000001,
+   kClassHasExplicitCtor        = 0x00000010,
+   kClassHasImplicitCtor        = 0x00000020,
+   kClassHasCtor                = 0x00000030,
+   kClassHasDefaultCtor         = 0x00000040,
+   kClassHasAssignOpr           = 0x00000080,
+   kClassHasExplicitDtor        = 0x00000100,
+   kClassHasImplicitDtor        = 0x00000200,
+   kClassHasDtor                = 0x00000300,
+   kClassHasVirtual             = 0x00001000,
+   kClassIsAbstract             = 0x00002000,
+   kClassIsAggregate            = 0x00004000,
+   kClassIsTriviallyRelocatable = 0x00008000
 };
+// clang-format on
 
 enum ERefTypeValues {
    kParaNormal     = 0,     // not used

core/meta/src/TClass.cxx

@@ -95,6 +95,7 @@ In order to access the name of a class within the ROOT type system, the method T
 #include <sstream>
 #include <string>
 #include <map>
+#include <mutex>
 #include <typeinfo>
 #include <cmath>
 #include <cassert>
@@ -4378,6 +4379,39 @@ void TClass::MakeCustomMenuList()
    delete methodList;
 }
 
+////////////////////////////////////////////////////////////////////////////////
+/// Return kTRUE if an object of this class can be relocated to a new address
+/// with a raw memory copy, i.e. without running a move or copy constructor --
+/// trivial relocatability in the C++26 sense ([class.prop]), as answered by the
+/// interpreter. Every trivially copyable class is trivially relocatable, but not
+/// every trivially relocatable class is trivially copyable: e.g. a polymorphic
+/// class whose bases and members are all trivially relocatable qualifies too.
+///
+/// A kTRUE answer rules out both a resource being freed twice or from the wrong
+/// address (the failure mode of root-project/root#20882) and a non-trivial copy
+/// constructor being skipped. It is still not a proof that a raw memory copy
+/// preserves the class' semantics:
+/// ~~~ {.cpp}
+/// 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. Such a class is relocated the way it always has been.
+///
+/// An emulated class, described only by a TStreamerInfo, gets the conservative
+/// answer since its members can be anything -- e.g. the std::string of an
+/// emulated pair<string,double>.
+
+Bool_t TClass::IsTriviallyRelocatable() const
+{
+   const Long_t classProperty = ClassProperty();
+   // No kClassIsValid means no interpreter information at all (emulated class,
+   // forward declaration, ...), so assume the worst.
+   if (!(classProperty & kClassIsValid))
+      return kFALSE;
+   return (classProperty & kClassIsTriviallyRelocatable) != 0;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 /// Register the fact that an object was moved from the memory location
 /// 'arenaFrom' to the memory location 'arenaTo'.
@@ -4388,6 +4422,31 @@ void TClass::Move(void *arenaFrom, void *arenaTo) const
    // constructor), this function should also perform the data move.
    // For now we just information the repository.
 
+   // This only records the new address; a caller that relocated the data with a
+   // raw memory copy silently corrupts the types that do not support it, so warn
+   // about those rather than let it surface later as an obscure crash (typically
+   // an invalid free). A caller that did run a real move or copy constructor can
+   // ignore the message -- we have no way to tell the two apart here. The in-tree
+   // callers, in TEmulatedCollectionProxy::Expand, already avoid the memcpy for
+   // these types, so this is aimed at external users of this public method.
+   if (!IsTriviallyRelocatable()) {
+      // Keyed by name rather than by 'this': a TClass can be deleted and another
+      // one allocated at the same address, which would silence the message.
+      static std::mutex sMoveDiagMutex;
+      static std::set<std::string> sMoveDiagDone;
+      bool firstTime = false;
+      {
+         std::lock_guard<std::mutex> guard(sMoveDiagMutex);
+         firstTime = sMoveDiagDone.emplace(GetName()).second;
+      }
+      if (firstTime)
+         Error("Move",
+               "Objects of type %s are not trivially relocatable, i.e. can not be relocated with a raw memory copy. "
+               "TClass::Move does not move the data itself (here from %p to %p), so if the caller relocated it that "
+               "way the objects are now corrupted.",
+               GetName(), arenaFrom, arenaTo);
+   }
+
    if ((GetState() <= kEmulated) && !fCollectionProxy) {
       MoveAddressInRepository("TClass::Move",arenaFrom,arenaTo,this);
    }

core/metacling/src/TClingClassInfo.cxx

@@ -199,6 +199,13 @@ long TClingClassInfo::ClassProperty() const
       // according to the C++ standard, being a POD implies being an aggregate
       property |= kClassIsAggregate;
    }
+   if (CRD->hasDefinition() && fInterp->getSema().IsCXXTriviallyRelocatableType(*CRD)) {
+      // Trivial relocatability in the C++26 sense ([class.prop]), as computed by
+      // Sema. This is more accurate than isTriviallyCopyable(): every trivially
+      // copyable class is trivially relocatable, but not vice versa -- e.g. a
+      // polymorphic class whose bases and members are all trivially relocatable.
+      property |= kClassIsTriviallyRelocatable;
+   }
    return property;
 }
 

core/metacling/test/TClingTests.cxx

@@ -318,20 +318,49 @@ struct ClassIsAggregate {
     int x;
     double y;
 };
+
+struct ClassIsTriviallyRelocatable {
+    int x;
+    double y;
+};
+
+// not trivially copyable because of the virtual function, but still trivially
+// relocatable in the C++26 sense
+struct ClassIsTriviallyRelocatablePolymorphic {
+    virtual ~ClassIsTriviallyRelocatablePolymorphic() = default;
+    int x;
+};
+
+// a non-trivial copy constructor is enough to lose trivial relocatability, even
+// though the destructor stays trivial
+struct ClassIsNotTriviallyRelocatable {
+    int x;
+    ClassIsNotTriviallyRelocatable(const ClassIsNotTriviallyRelocatable &other) : x(other.x) {}
+};
                           )cpp");
 
+   // clang-format off
    const std::vector<std::pair<std::string, Long_t>> classNPPairs{
-      {"ClassHasImplicitCtor", kClassHasImplicitCtor}, {"ClassHasExplicitCtor", kClassHasExplicitCtor},
-      {"ClassHasExplicitDtor", kClassHasExplicitDtor}, {"ClassHasImplicitDtor", kClassHasImplicitDtor},
-      {"ClassHasDefaultCtor", kClassHasDefaultCtor},   {"ClassHasDefaultCtor", kClassIsValid},
-      {"ClassIsAbstract", kClassIsAbstract},           {"ClassHasVirtual", kClassHasVirtual},
-      {"ClassHasAssignOpr", kClassHasAssignOpr},       {"ClassIsAggregate", kClassIsAggregate}};
+      {"ClassHasImplicitCtor", kClassHasImplicitCtor},           {"ClassHasExplicitCtor", kClassHasExplicitCtor},
+      {"ClassHasExplicitDtor", kClassHasExplicitDtor},           {"ClassHasImplicitDtor", kClassHasImplicitDtor},
+      {"ClassHasDefaultCtor", kClassHasDefaultCtor},             {"ClassHasDefaultCtor", kClassIsValid},
+      {"ClassIsAbstract", kClassIsAbstract},                     {"ClassHasVirtual", kClassHasVirtual},
+      {"ClassHasAssignOpr", kClassHasAssignOpr},                 {"ClassIsAggregate", kClassIsAggregate},
+      {"ClassIsTriviallyRelocatable", kClassIsTriviallyRelocatable},
+      {"ClassIsTriviallyRelocatablePolymorphic", kClassIsTriviallyRelocatable}};
+   // clang-format on
 
    for (auto &[clName, clPropRef] : classNPPairs) {
       auto cl = TClass::GetClass(clName.c_str());
       const auto prop = gInterpreter->ClassInfo_ClassProperty(cl->GetClassInfo());
       EXPECT_TRUE(prop & clPropRef) << "Error checking property for class " << clName;
    }
+
+   // A trivial destructor is not enough: the copy constructor matters too.
+   auto notTrivial = TClass::GetClass("ClassIsNotTriviallyRelocatable");
+   const auto notTrivialProp = gInterpreter->ClassInfo_ClassProperty(notTrivial->GetClassInfo());
+   EXPECT_FALSE(notTrivialProp & kClassIsTriviallyRelocatable);
+   EXPECT_FALSE(notTrivialProp & kClassHasDtor);
 }
 
 // #12108

io/io/inc/TEmulatedCollectionProxy.h

@@ -117,7 +117,7 @@ class TEmulatedCollectionProxy : public TGenCollectionProxy  {
    void Shrink(UInt_t nCurr, UInt_t left, Bool_t force);
 
    // Expand the container
-   void Expand(UInt_t nCurr, UInt_t left);
+   void Expand(UInt_t nCurr, UInt_t left, Bool_t force);
 
 private:
    TEmulatedCollectionProxy &operator=(const TEmulatedCollectionProxy &); // Not implemented.

io/io/inc/TGenCollectionProxy.h

@@ -318,6 +318,14 @@ class TGenCollectionProxy
    EnvironBase_t*fEnv;       ///< Address of the currently proxied object
    int           fValOffset; ///< Offset from key to value (in maps)
    int           fValDiff;   ///< Offset between two consecutive value_types (memory layout).
+
+   /// Byte offset of the n-th element. fValDiff is an int and the element counts
+   /// are UInt_t, so a plain product would be computed in 32 bits and wrap once
+   /// the collection data grows past 4 GiB -- which is reached well within the
+   /// UInt_t element counts this interface supports whenever the elements are
+   /// bigger than one byte.
+   std::size_t ElementOffset(std::size_t n) const { return n * static_cast<std::size_t>(fValDiff); }
+
    Proxies_t     fProxyList; ///< Stack of recursive proxies
    Proxies_t     fProxyKept; ///< Optimization: Keep proxies once they were created
    Staged_t      fStaged;    ///< Optimization: Keep staged array once they were created