From 5defd6ad056f86a3141fc95c3cf0b1eb80c26eb3 Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Sat, 24 Oct 2026 11:01:33 +0200 Subject: [PATCH 1/5] [io] Reject negative collection sizes when reading The element count of an STL collection is stored as an Int_t but passed on to TVirtualCollectionProxy::Allocate(UInt_t) or std::vector::resize. When the count comes from corrupt data and is negative, it wraps to billions of elements: in an LHCb DST, a count of 0xa2c35db6 made ROOT allocate and value-initialise 43.7 GB before the reading code could notice anything, getting grid jobs OOM-killed, or throw std::bad_alloc or std::length_error out of TBranch::GetEntry. TBufferFile already rejects such counts for arrays and TBranchElement for split collections, but the collection streaming actions and the collection proxies, including the emulated ones used when reading without a dictionary, did not. Report an error and read the collection as empty instead. The rest of the object is then read from a wrong position until the next byte count check, if any, resynchronises the buffer; it is no worse than reading corrupt data was before. The generic collection streamers used to skip negative sizes silently, either reading the collection as empty or leaving its previous content; they now report them and read the collection as empty too. TConvertClonesArrayToProxy takes the absolute value of the count for backward compatibility, which is undefined for INT_MIN; keep that one negative so that it is rejected too. The check lives in a private header of core/meta, so that the streamers of libCore containers can share it. Assisted-by: claude-code:claude-opus-5-5 --- core/meta/CMakeLists.txt | 2 + core/meta/res/ROOT/CheckCollectionSize.hxx | 44 +++++ io/io/src/TContainerConverters.cxx | 7 +- io/io/src/TEmulatedCollectionProxy.cxx | 4 + io/io/src/TEmulatedMapProxy.cxx | 5 +- io/io/src/TGenCollectionStreamer.cxx | 12 ++ io/io/src/TStreamerInfoActions.cxx | 29 +++ io/io/src/TStreamerInfoReadBuffer.cxx | 5 + io/io/test/TBufferFileTests.cxx | 215 +++++++++++++++++++++ 9 files changed, 321 insertions(+), 2 deletions(-) create mode 100644 core/meta/res/ROOT/CheckCollectionSize.hxx diff --git a/core/meta/CMakeLists.txt b/core/meta/CMakeLists.txt index 8e010343a9a34..efb02a7ae7f76 100644 --- a/core/meta/CMakeLists.txt +++ b/core/meta/CMakeLists.txt @@ -91,6 +91,8 @@ target_sources(Core PRIVATE target_include_directories(Core PUBLIC $ + PRIVATE + res ) ROOT_INSTALL_HEADERS() diff --git a/core/meta/res/ROOT/CheckCollectionSize.hxx b/core/meta/res/ROOT/CheckCollectionSize.hxx new file mode 100644 index 0000000000000..40bee9e3b21fa --- /dev/null +++ b/core/meta/res/ROOT/CheckCollectionSize.hxx @@ -0,0 +1,44 @@ +/************************************************************************* + * Copyright (C) 1995-2026, Rene Brun and Fons Rademakers. * + * All rights reserved. * + * * + * For the licensing terms see $ROOTSYS/LICENSE. * + * For the list of contributors see $ROOTSYS/README/CREDITS. * + *************************************************************************/ + +#ifndef ROOT_CheckCollectionSize +#define ROOT_CheckCollectionSize + +// Private header shared by libCore and libRIO. + +#include "RtypesCore.h" +#include "TBuffer.h" +#include "TError.h" + +namespace ROOT { +namespace Internal { + +//////////////////////////////////////////////////////////////////////////////// +/// Validate the number of elements of a collection that was just read from `buf`. +/// +/// The collection size is stored as an Int_t but consumed as an unsigned value +/// (TVirtualCollectionProxy::Allocate, std::vector::resize), so a negative value +/// coming from corrupt data would turn into a request for billions of elements. +/// Report the problem from `location` and read the collection as empty instead. +inline Int_t CheckCollectionSize(TBuffer &buf, Int_t nElements, const char *location, const char *collectionName) +{ + if (R__unlikely(nElements < 0)) { + ::Error(location, + "Corrupt data: the collection of type %s read from %s has a negative number of elements (%d); " + "reading it as empty.", + collectionName ? collectionName : "(unknown)", + buf.GetParent() ? buf.GetParent()->GetName() : "memory/socket", nElements); + return 0; + } + return nElements; +} + +} // namespace Internal +} // namespace ROOT + +#endif diff --git a/io/io/src/TContainerConverters.cxx b/io/io/src/TContainerConverters.cxx index 8e25771e4e812..238f066f8fe3d 100644 --- a/io/io/src/TContainerConverters.cxx +++ b/io/io/src/TContainerConverters.cxx @@ -31,7 +31,9 @@ #include "TError.h" #include "TGenCollectionStreamer.h" #include "TClassStreamer.h" +#include "ROOT/CheckCollectionSize.hxx" #include +#include namespace { const Int_t kMapOffset = 2; @@ -201,7 +203,10 @@ void TConvertClonesArrayToProxy::operator()(TBuffer &b, void *pmember, Int_t siz } b >> nobjects; - if (nobjects < 0) nobjects = -nobjects; // still there for backward compatibility + if (nobjects < 0 && nobjects != std::numeric_limits::min()) + nobjects = -nobjects; // still there for backward compatibility + nobjects = ROOT::Internal::CheckCollectionSize(b, nobjects, "TConvertClonesArrayToProxy::operator()", + fCollectionClass->GetName()); b >> dummy; // fLowerBound is ignored if (cl != subinfo->GetClass()) { Error("TClonesArray::Conversion to vector","Bad class"); diff --git a/io/io/src/TEmulatedCollectionProxy.cxx b/io/io/src/TEmulatedCollectionProxy.cxx index 883dab626fd3a..af15c32952a01 100644 --- a/io/io/src/TEmulatedCollectionProxy.cxx +++ b/io/io/src/TEmulatedCollectionProxy.cxx @@ -35,6 +35,7 @@ the class TEmulatedMapProxy. #include "TVirtualMutex.h" // For R__LOCKGUARD #include "TInterpreter.h" // For gInterpreterMutex +#include "ROOT/CheckCollectionSize.hxx" // // Utility function to allow the creation of a TClass for a std::pair without @@ -665,6 +666,7 @@ void TEmulatedCollectionProxy::ReadBuffer(TBuffer &b, void *obj) TPushPop env(this,obj); int nElements = 0; b >> nElements; + nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedCollectionProxy::ReadBuffer", fName.c_str()); if ( fEnv->fObject ) { Resize(nElements,true); } @@ -679,6 +681,8 @@ void TEmulatedCollectionProxy::Streamer(TBuffer &b) if ( b.IsReading() ) { //Read mode int nElements = 0; b >> nElements; + nElements = + ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedCollectionProxy::Streamer", fName.c_str()); if ( fEnv->fObject ) { Resize(nElements,true); } diff --git a/io/io/src/TEmulatedMapProxy.cxx b/io/io/src/TEmulatedMapProxy.cxx index d1ec2d546bd55..c8f5de5aba1da 100644 --- a/io/io/src/TEmulatedMapProxy.cxx +++ b/io/io/src/TEmulatedMapProxy.cxx @@ -35,6 +35,7 @@ is separated from the TEmulatedProxy. #include "TClassEdit.h" #include "TStreamerInfo.h" #include "TError.h" +#include "ROOT/CheckCollectionSize.hxx" TEmulatedMapProxy::TEmulatedMapProxy(const TEmulatedMapProxy& copy) : TEmulatedCollectionProxy(copy) @@ -226,6 +227,7 @@ void TEmulatedMapProxy::ReadBuffer(TBuffer &b, void *obj) TPushPop env(this,obj); int nElements = 0; b >> nElements; + nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedMapProxy::ReadBuffer", fName.c_str()); if ( fEnv->fObject ) { Resize(nElements,true); } @@ -238,8 +240,9 @@ void TEmulatedMapProxy::Streamer(TBuffer &b) { // TClassStreamer IO overload. if ( b.IsReading() ) { //Read mode - UInt_t nElements = 0; + Int_t nElements = 0; b >> nElements; + nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedMapProxy::Streamer", fName.c_str()); if ( fEnv->fObject ) { Resize(nElements,true); } diff --git a/io/io/src/TGenCollectionStreamer.cxx b/io/io/src/TGenCollectionStreamer.cxx index b8918ec5de02a..a832a9fa568cc 100644 --- a/io/io/src/TGenCollectionStreamer.cxx +++ b/io/io/src/TGenCollectionStreamer.cxx @@ -29,6 +29,7 @@ size(), clear(), resize(). resize() may be a void operation. #include "TStreamerInfo.h" #include "TStreamerElement.h" #include "TVirtualCollectionIterators.h" +#include "ROOT/CheckCollectionSize.hxx" #include @@ -1183,6 +1184,8 @@ void TGenCollectionStreamer::ReadBufferVectorPrimitives(TBuffer &b, void *obj, c { int nElements = 0; b >> nElements; + nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitives", + fName.c_str()); fResize(obj,nElements); if (onFileClass) { @@ -1198,6 +1201,8 @@ void TGenCollectionStreamer::ReadBufferVectorPrimitivesFloat16(TBuffer &b, void { int nElements = 0; b >> nElements; + nElements = ROOT::Internal::CheckCollectionSize( + b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitivesFloat16", fName.c_str()); fResize(obj,nElements); if (onFileClass) { @@ -1213,6 +1218,8 @@ void TGenCollectionStreamer::ReadBufferVectorPrimitivesDouble32(TBuffer &b, void { int nElements = 0; b >> nElements; + nElements = ROOT::Internal::CheckCollectionSize( + b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitivesDouble32", fName.c_str()); fResize(obj,nElements); if (onFileClass) { @@ -1318,6 +1325,8 @@ void TGenCollectionStreamer::ReadBufferGeneric(TBuffer &b, void *obj, const TCla int nElements = 0; b >> nElements; + nElements = + ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::ReadBufferGeneric", fName.c_str()); if (nElements == 0) { if (obj) { @@ -1402,6 +1411,7 @@ void TGenCollectionStreamer::Streamer(TBuffer &b) if (b.IsReading()) { //Read mode int nElements = 0; b >> nElements; + nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::Streamer", fName.c_str()); if (fEnv->fObject) { TGenCollectionProxy::Clear("force"); } @@ -1480,6 +1490,8 @@ void TGenCollectionStreamer::StreamerAsMap(TBuffer &b) if (b.IsReading()) { //Read mode int nElements = 0; b >> nElements; + nElements = + ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::StreamerAsMap", fName.c_str()); if (fEnv->fObject) { TGenCollectionProxy::Clear("force"); } diff --git a/io/io/src/TStreamerInfoActions.cxx b/io/io/src/TStreamerInfoActions.cxx index 08468706c7f67..a2659caf88853 100644 --- a/io/io/src/TStreamerInfoActions.cxx +++ b/io/io/src/TStreamerInfoActions.cxx @@ -22,6 +22,7 @@ #include "TMemberStreamer.h" #include "TClassEdit.h" #include "TVirtualCollectionIterators.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TProcessID.h" #include "TFile.h" @@ -784,6 +785,8 @@ namespace TStreamerInfoActions TVirtualCollectionProxy::TPushPop helper( oldProxy, (char*)addr ); Int_t nobjects; buf.ReadInt(nobjects); + nobjects = ROOT::Internal::CheckCollectionSize( + buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", config->fTypeName); void* alternative = oldProxy->Allocate(nobjects,true); if (nobjects) { TActionSequence *actions = oldProxy->GetReadMemberWiseActions( vClVersion ); @@ -816,6 +819,8 @@ namespace TStreamerInfoActions TVirtualCollectionProxy::TPushPop helper( oldProxy, (char*)addr ); Int_t nobjects; buf.ReadInt(nobjects); + nobjects = ROOT::Internal::CheckCollectionSize( + buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", config->fTypeName); void* env = oldProxy->Allocate(nobjects,true); if (nobjects || vers < 7 ) { @@ -856,6 +861,8 @@ namespace TStreamerInfoActions for(; objfTypeName); TVirtualCollectionProxy::TPushPop helper( oldProxy, (char*)obj ); void* alternative = oldProxy->Allocate(nobjects,true); if (nobjects) { @@ -893,6 +900,8 @@ namespace TStreamerInfoActions TVirtualCollectionProxy::TPushPop helper( oldProxy, (char*)obj ); Int_t nobjects; buf.ReadInt(nobjects); + nobjects = ROOT::Internal::CheckCollectionSize( + buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseSameClass", config->fTypeName); void* env = oldProxy->Allocate(nobjects,true); if (nobjects || vers < 7 ) { @@ -930,6 +939,8 @@ namespace TStreamerInfoActions TVirtualCollectionProxy::TPushPop helper( newProxy, (char*)addr ); Int_t nobjects; buf.ReadInt(nobjects); + nobjects = ROOT::Internal::CheckCollectionSize( + buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseChangedClass", config->fTypeName); void* alternative = newProxy->Allocate(nobjects,true); if (nobjects) { TActionSequence *actions = newProxy->GetConversionReadMemberWiseActions( oldProxy->GetValueClass(), vClVersion ); @@ -979,6 +990,8 @@ namespace TStreamerInfoActions TVirtualCollectionProxy::TPushPop helper( newProxy, (char*)obj ); Int_t nobjects; buf.ReadInt(nobjects); + nobjects = ROOT::Internal::CheckCollectionSize( + buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseChangedClass", config->fTypeName); void* alternative = newProxy->Allocate(nobjects,true); if (nobjects) { TActionSequence *actions = newProxy->GetConversionReadMemberWiseActions( oldProxy->GetValueClass(), vClVersion ); @@ -2232,6 +2245,8 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadCollectionBasicType", + config->fTypeName); vec->resize(nvalues); #ifdef R__VISUAL_CPLUSPLUS @@ -2277,6 +2292,8 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadCollectionFloat16", + config->fTypeName); vec->resize(nvalues); #ifdef R__VISUAL_CPLUSPLUS @@ -2321,6 +2338,8 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadCollectionDouble32", + config->fTypeName); vec->resize(nvalues); #ifdef R__VISUAL_CPLUSPLUS @@ -2367,6 +2386,8 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize( + buf, nvalues, "TStreamerInfoActions::ConvertCollectionBasicType", config->fTypeName); vec->resize(nvalues); From *temp = new From[nvalues]; @@ -2394,6 +2415,8 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize( + buf, nvalues, "TStreamerInfoActions::ConvertCollectionBasicType", config->fTypeName); vec->resize(nvalues); From *temp = new From[nvalues]; @@ -2420,6 +2443,8 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ConvertCollectionDouble32", + config->fTypeName); vec->resize(nvalues); Double32_t *temp = new Double32_t[nvalues]; @@ -2766,6 +2791,8 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadNumericalCollection", + config->fTypeName); void* alternative = newProxy->Allocate(nvalues,true); if (nvalues) { char startbuf[TVirtualCollectionProxy::fgIteratorArenaSize]; @@ -3364,6 +3391,8 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); + nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadNumericalCollection", + config->fTypeName); void* alternative = newProxy->Allocate(nvalues,true); if (nvalues) { char startbuf[TVirtualCollectionProxy::fgIteratorArenaSize]; diff --git a/io/io/src/TStreamerInfoReadBuffer.cxx b/io/io/src/TStreamerInfoReadBuffer.cxx index 8135f9416fc82..0937717bf9372 100644 --- a/io/io/src/TStreamerInfoReadBuffer.cxx +++ b/io/io/src/TStreamerInfoReadBuffer.cxx @@ -25,6 +25,7 @@ #include "TVirtualArray.h" #include "TVirtualObject.h" #include "ThreadLocalStorage.h" +#include "ROOT/CheckCollectionSize.hxx" // GetCurrentElement. // Currently only used by TRef::Streamer. @@ -1193,6 +1194,8 @@ Int_t TStreamerInfo::ReadBuffer(TBuffer &b, const T &arr, TVirtualCollectionProxy::TPushPop helper( newProxy, cont ); Int_t nobjects; b >> nobjects; + nobjects = ROOT::Internal::CheckCollectionSize(b, nobjects, "TStreamerInfo::ReadBuffer", + cle->GetName()); env = newProxy->Allocate(nobjects,true); if (!nobjects && (vers>=7)) { // Do nothing but in version 6 of TStreamerInfo and below, @@ -1303,6 +1306,8 @@ Int_t TStreamerInfo::ReadBuffer(TBuffer &b, const T &arr, TVirtualCollectionProxy::TPushPop helper( newProxy, obj ); Int_t nobjects; b >> nobjects; + nobjects = ROOT::Internal::CheckCollectionSize(b, nobjects, "TStreamerInfo::ReadBuffer", + cle->GetName()); void* env = newProxy->Allocate(nobjects,true); if (!nobjects && (vers>=7)) { // Do nothing but in version 6 of TStreamerInfo and below, diff --git a/io/io/test/TBufferFileTests.cxx b/io/io/test/TBufferFileTests.cxx index 891f4fcf0d06d..c550e1fa410a3 100644 --- a/io/io/test/TBufferFileTests.cxx +++ b/io/io/test/TBufferFileTests.cxx @@ -1,10 +1,19 @@ #include "gtest/gtest.h" +#include "ROOT/TestSupport.hxx" + #include "TBufferFile.h" #include "TMemFile.h" #include "TClass.h" +#include "TClonesArray.h" +#include "TContainerConverters.h" #include "TInterpreter.h" #include "TKey.h" +#include "TNamed.h" +#include "TVirtualCollectionProxy.h" +#include +#include +#include #include #include @@ -49,3 +58,209 @@ TEST(TBufferFile, ForeignZeroVersionClass) EXPECT_NE(f.Get("s1"), nullptr); EXPECT_EQ(f.GetKey("s0")->GetObjlen(), 10); } + +// A collection size read from corrupt data must not be used to allocate memory. +#define CORRUPTINNER \ + struct CorruptInner { \ + int fA = 7; \ + char fPad[1024] = {}; \ + }; +#define CORRUPTOUTER \ + struct CorruptOuter { \ + std::vector fObjs; \ + std::vector fInts; \ + }; +// Collections inside the elements of a member-wise collection are read by TStreamerInfo::ReadBuffer. +#define CORRUPTNESTED \ + struct CorruptNested { \ + std::vector fOuters; \ + }; +CORRUPTINNER +CORRUPTOUTER +CORRUPTNESTED + +namespace { + +// The element count of RawEvent::m_banks found in a corrupt LHCb file; negative as an Int_t. +constexpr unsigned int kCorruptSize = 0xa2c35db6; + +/// Position of the big-endian Int_t equal to `expected` in `data`, which must occur exactly once. +/// Returns data.size() if it does not. +std::size_t FindInt(const std::vector &data, unsigned int expected) +{ + const char pattern[4] = {char(expected >> 24), char(expected >> 16), char(expected >> 8), char(expected)}; + const auto it = std::search(data.begin(), data.end(), pattern, pattern + 4); + if (it == data.end() || std::search(it + 1, data.end(), pattern, pattern + 4) != data.end()) { + ADD_FAILURE() << "expected exactly one occurrence of " << expected << " in the serialized data"; + return data.size(); + } + return it - data.begin(); +} + +/// Overwrite the big-endian Int_t equal to `expected` in `data` with `value`. +void CorruptInt(std::vector &data, unsigned int expected, unsigned int value) +{ + const auto pos = FindInt(data, expected); + if (pos == data.size()) + return; + for (int i = 0; i < 4; ++i) + data[pos + i] = char(value >> (8 * (3 - i))); +} + +std::vector Write(const void *obj, TClass *cl) +{ + TBufferFile buf(TBuffer::kWrite); + buf.WriteObjectAny(obj, cl); + return std::vector(buf.Buffer(), buf.Buffer() + buf.Length()); +} + +void *Read(std::vector &data, TClass *cl) +{ + TBufferFile buf(TBuffer::kRead, data.size(), data.data(), /*adopt=*/false); + return buf.ReadObjectAny(cl); +} + +template +std::vector Write(const T &obj) +{ + return Write(&obj, TClass::GetClass()); +} + +template +std::unique_ptr Read(std::vector &data) +{ + return std::unique_ptr(static_cast(Read(data, TClass::GetClass()))); +} + +/// Number of elements of the collection of type `cl` at `obj`. +UInt_t CollectionSize(TClass *cl, void *obj) +{ + TVirtualCollectionProxy::TPushPop helper(cl->GetCollectionProxy(), obj); + return cl->GetCollectionProxy()->Size(); +} + +class CorruptCollectionSize : public ::testing::Test { +protected: + static constexpr int kNObjs = 0x123; + static constexpr int kNInts = 0x321; + + std::vector fData; + + static void SetUpTestSuite() + { + gInterpreter->Declare(TO_LITERAL(CORRUPTINNER)); + gInterpreter->Declare(TO_LITERAL(CORRUPTOUTER)); + gInterpreter->Declare(TO_LITERAL(CORRUPTNESTED)); + } + + void SetUp() override + { + CorruptOuter outer; + outer.fObjs.resize(kNObjs); + outer.fInts.assign(kNInts, 1); + fData = Write(outer); + } +}; + +} // namespace + +TEST_F(CorruptCollectionSize, MemberWise) +{ + CorruptInt(fData, kNObjs, kCorruptSize); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", "negative number of elements", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto outer = Read(fData); + ASSERT_NE(outer, nullptr); + EXPECT_TRUE(outer->fObjs.empty()); + EXPECT_EQ(outer->fInts.size(), kNInts); +} + +TEST_F(CorruptCollectionSize, BasicType) +{ + CorruptInt(fData, kNInts, kCorruptSize); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TStreamerInfoActions::ReadCollectionBasicType", "negative number of elements", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto outer = Read(fData); + ASSERT_NE(outer, nullptr); + EXPECT_EQ(outer->fObjs.size(), kNObjs); + EXPECT_TRUE(outer->fInts.empty()); +} + +// Collections without a dictionary are read through TEmulatedCollectionProxy and TEmulatedMapProxy. +TEST_F(CorruptCollectionSize, Emulated) +{ + for (auto [name, location] : {std::make_pair("vector", "TEmulatedCollectionProxy::ReadBuffer"), + std::make_pair("map", "TEmulatedMapProxy::ReadBuffer")}) { + SCOPED_TRACE(name); + TClass *cl = TClass::GetClass(name); + ASSERT_NE(cl, nullptr); + ASSERT_TRUE(cl->GetCollectionProxy()->GetProperties() & TVirtualCollectionProxy::kIsEmulated); + void *obj = cl->New(); + { + // Emulated maps are stored like vectors of pairs, so the duplicate keys do not matter. + TVirtualCollectionProxy::TPushPop helper(cl->GetCollectionProxy(), obj); + cl->GetCollectionProxy()->Commit(cl->GetCollectionProxy()->Allocate(kNObjs, true)); + } + ASSERT_EQ(CollectionSize(cl, obj), kNObjs); + auto data = Write(obj, cl); + cl->Destructor(obj); + CorruptInt(data, kNObjs, kCorruptSize); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, location, "negative number of elements", /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + void *read = Read(data, cl); + ASSERT_NE(read, nullptr); + EXPECT_EQ(CollectionSize(cl, read), 0u); + cl->Destructor(read); + } +} + +TEST_F(CorruptCollectionSize, Nested) +{ + CorruptNested nested; + nested.fOuters.resize(1); + nested.fOuters[0].fObjs.resize(kNObjs); + auto data = Write(nested); + CorruptInt(data, kNObjs, kCorruptSize); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TStreamerInfo::ReadBuffer", "negative number of elements", /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto read = Read(data); + ASSERT_NE(read, nullptr); + ASSERT_EQ(read->fOuters.size(), 1u); + EXPECT_TRUE(read->fOuters[0].fObjs.empty()); +} + +// A TClonesArray on file is read into an STL collection by TConvertClonesArrayToProxy, which takes the +// absolute value of negative sizes. +TEST_F(CorruptCollectionSize, ClonesArrayToProxy) +{ + TClonesArray clones("TNamed", kNObjs); + for (int i = 0; i < kNObjs; ++i) + clones.ConstructedAt(i); + TBufferFile wbuf(TBuffer::kWrite); + clones.Streamer(wbuf); + std::vector data(wbuf.Buffer(), wbuf.Buffer() + wbuf.Length()); + CorruptInt(data, kNObjs, std::numeric_limits::min()); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TConvertClonesArrayToProxy::operator()", "negative number of elements", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + TClass *cl = TClass::GetClass("vector"); + ASSERT_NE(cl, nullptr); + TConvertClonesArrayToProxy convert(cl->GetCollectionProxy(), /*isPointer=*/false, /*isPrealloc=*/false); + void *read = cl->New(); + TBufferFile rbuf(TBuffer::kRead, data.size(), data.data(), /*adopt=*/false); + convert(rbuf, read); + EXPECT_EQ(CollectionSize(cl, read), 0u); + cl->Destructor(read); +} From a1aaa668961ea0cb91d14faf0677271b7550eda8 Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Sat, 24 Oct 2026 11:16:43 +0200 Subject: [PATCH 2/5] [io] Bound collection sizes by the remaining bytes A corrupt collection size that is positive but larger than the data can hold is not caught by the negative size check: the collection is resized to up to 2^31 elements, and when reading member-wise the streaming actions then run past the end of the buffer. When each element is known to occupy at least one byte on file, a size larger than the number of bytes left in the buffer is impossible, so report it and read the collection as empty. This holds for collections of basic types, strings and pointers written with an object tag, and for member-wise collections whose value class has, in its layout on file, at least one data member of such a type. It does not hold for objects of class type, which can be written in zero bytes by a custom streamer, including those pointed to by a `//->` member, nor for data members with a custom member streamer, nor for pointers to fundamental types, which collection proxies do not stream at all. Those are left unchecked. The check is restricted to TBufferFile, and its cost is only paid for sizes that exceed the remaining bytes. Bounding by one byte per element rather than by the actual size of an element keeps the check simple; it still lets through sizes up to the size of the buffer, but no longer arbitrary ones. Assisted-by: claude-code:claude-opus-5-5 --- core/meta/res/ROOT/CheckCollectionSize.hxx | 78 +++++ io/io/src/ObjectWiseElementsUseBytes.h | 54 ++++ io/io/src/TContainerConverters.cxx | 6 +- io/io/src/TEmulatedCollectionProxy.cxx | 8 +- io/io/src/TEmulatedMapProxy.cxx | 9 +- io/io/src/TGenCollectionStreamer.cxx | 17 +- io/io/src/TStreamerInfoActions.cxx | 51 +++- io/io/src/TStreamerInfoReadBuffer.cxx | 10 +- io/io/test/TBufferFileTests.cxx | 326 +++++++++++++++++++++ 9 files changed, 528 insertions(+), 31 deletions(-) create mode 100644 io/io/src/ObjectWiseElementsUseBytes.h diff --git a/core/meta/res/ROOT/CheckCollectionSize.hxx b/core/meta/res/ROOT/CheckCollectionSize.hxx index 40bee9e3b21fa..3afea00414dc0 100644 --- a/core/meta/res/ROOT/CheckCollectionSize.hxx +++ b/core/meta/res/ROOT/CheckCollectionSize.hxx @@ -14,6 +14,9 @@ #include "RtypesCore.h" #include "TBuffer.h" #include "TError.h" +#include "TObjArray.h" +#include "TStreamerElement.h" +#include "TVirtualStreamerInfo.h" namespace ROOT { namespace Internal { @@ -38,6 +41,81 @@ inline Int_t CheckCollectionSize(TBuffer &buf, Int_t nElements, const char *loca return nElements; } +//////////////////////////////////////////////////////////////////////////////// +/// Validate the number of elements of a collection that was just read from `buf`. +/// +/// In addition to the checks of the overload above, reject sizes larger than +/// the number of bytes left in the buffer if each element is known to occupy +/// at least one byte on file. Such a size can only come from corrupt data, and +/// unlike a negative one it is not caught by Allocate or resize, which would +/// try to allocate and initialise up to 2^31 elements. +/// +/// \param[in] elementsUseBytes Callable returning whether each element occupies +/// at least one byte on file. It is only called for sizes larger than +/// the remaining bytes, so it does not slow down reading valid data. +template +inline Int_t CheckCollectionSize(TBuffer &buf, Int_t nElements, const char *location, const char *collectionName, + ElementsUseBytes &&elementsUseBytes) +{ + nElements = CheckCollectionSize(buf, nElements, location, collectionName); + const Long64_t remaining = (Long64_t)buf.BufferSize() - buf.Length(); + // TBufferText and its subclasses do not keep the data in the buffer. TBufferSQL inherits from TBufferFile + // without doing so either, but TTreeSQL does not support branches of collections or objects. + if (R__unlikely(nElements > remaining) && buf.InheritsFrom("TBufferFile") && elementsUseBytes()) { + ::Error(location, + "Corrupt data: the collection of type %s read from %s has %d elements, but only %lld bytes are left " + "in the buffer; reading it as empty.", + collectionName ? collectionName : "(unknown)", + buf.GetParent() ? buf.GetParent()->GetName() : "memory/socket", nElements, remaining); + return 0; + } + return nElements; +} + +//////////////////////////////////////////////////////////////////////////////// +/// Return whether each element of a collection streamed member-wise with the +/// on-file layout `info` occupies at least one byte on file. +/// +/// This is the case as soon as one of the data members is streamed as a +/// basic type or an array of them, a pointer written with an object tag, a +/// string or an STL collection. Members of class type are not considered: an +/// object can occupy zero bytes, e.g. with a custom streamer. Neither are +/// pointers marked `//->`, whose object is streamed in place. +inline bool MemberWiseElementsUseBytes(const TVirtualStreamerInfo *info) +{ + if (!info || !info->GetElements()) + return false; + for (const auto *obj : *info->GetElements()) { + const auto *element = dynamic_cast(obj); + // Artificial elements are set by I/O customization rules, they do not read from the buffer, and custom member + // streamers can read anything. + if (!element || dynamic_cast(element) || element->GetStreamer()) + continue; + const Int_t type = element->GetType(); + switch (type) { + case TVirtualStreamerInfo::kObjectP: + case TVirtualStreamerInfo::kTString: + case TVirtualStreamerInfo::kTObject: + case TVirtualStreamerInfo::kTNamed: + case TVirtualStreamerInfo::kAnyP: + case TVirtualStreamerInfo::kAnyPnoVT: + case TVirtualStreamerInfo::kSTLp: return true; + default: + if (type > TVirtualStreamerInfo::kBase && type < TVirtualStreamerInfo::kObject) { + // A basic type, an array of them (kOffsetL), or a pointer to an array of them (kOffsetP), which starts + // with a byte telling whether the array is present. + const Int_t basicType = type % TVirtualStreamerInfo::kOffsetL; + if (basicType >= TVirtualStreamerInfo::kChar && basicType <= TVirtualStreamerInfo::kFloat16 && + basicType != TVirtualStreamerInfo::kLegacyChar) + return true; + } else if (type >= TVirtualStreamerInfo::kSTL && type <= TVirtualStreamerInfo::kSTLstring) { + return true; + } + } + } + return false; +} + } // namespace Internal } // namespace ROOT diff --git a/io/io/src/ObjectWiseElementsUseBytes.h b/io/io/src/ObjectWiseElementsUseBytes.h new file mode 100644 index 0000000000000..010da27784fb4 --- /dev/null +++ b/io/io/src/ObjectWiseElementsUseBytes.h @@ -0,0 +1,54 @@ +/************************************************************************* + * Copyright (C) 1995-2026, Rene Brun and Fons Rademakers. * + * All rights reserved. * + * * + * For the licensing terms see $ROOTSYS/LICENSE. * + * For the list of contributors see $ROOTSYS/README/CREDITS. * + *************************************************************************/ + +#ifndef ROOT_ObjectWiseElementsUseBytes +#define ROOT_ObjectWiseElementsUseBytes + +#include "TDataType.h" +#include "TDictionary.h" +#include "TGenCollectionProxy.h" + +namespace ROOT { +namespace Internal { + +//////////////////////////////////////////////////////////////////////////////// +/// Return whether an element described by `value` occupies at least one byte on +/// file when streamed object-wise by a collection proxy. +/// +/// Objects of class type can occupy zero bytes, e.g. with a custom streamer, and +/// pointers to fundamental types are not streamed at all. +inline bool ObjectWiseElementUsesBytes(const TGenCollectionProxy::Value *value) +{ + if (!value) + return false; + switch (value->fCase) { + case kIsFundamental: + case kIsEnum: + // These kinds are not supported, and are not streamed. + return value->fKind != kchar && value->fKind != kNoType_t && value->fKind != kOther_t; + case TGenCollectionProxy::kBIT_ISSTRING: + case 0U | kIsPointer | kIsClass: + case 0U | kIsPointer | TGenCollectionProxy::kBIT_ISSTRING: + case 0U | kIsPointer | TGenCollectionProxy::kBIT_ISTSTRING | kIsClass: return true; + default: return false; + } +} + +//////////////////////////////////////////////////////////////////////////////// +/// Return whether each element of a collection proxy with the given key (for +/// maps, nullptr otherwise) and value descriptors occupies at least one byte on +/// file when streamed object-wise. +inline bool ObjectWiseElementsUseBytes(const TGenCollectionProxy::Value *key, const TGenCollectionProxy::Value *value) +{ + return ObjectWiseElementUsesBytes(key) || ObjectWiseElementUsesBytes(value); +} + +} // namespace Internal +} // namespace ROOT + +#endif diff --git a/io/io/src/TContainerConverters.cxx b/io/io/src/TContainerConverters.cxx index 238f066f8fe3d..6cd437c3c4d56 100644 --- a/io/io/src/TContainerConverters.cxx +++ b/io/io/src/TContainerConverters.cxx @@ -205,8 +205,12 @@ void TConvertClonesArrayToProxy::operator()(TBuffer &b, void *pmember, Int_t siz b >> nobjects; if (nobjects < 0 && nobjects != std::numeric_limits::min()) nobjects = -nobjects; // still there for backward compatibility + // Without kBypassStreamer, each element starts with a byte telling whether it is present. nobjects = ROOT::Internal::CheckCollectionSize(b, nobjects, "TConvertClonesArrayToProxy::operator()", - fCollectionClass->GetName()); + fCollectionClass->GetName(), [&] { + return !objdummy.TestBit(TClonesArray::kBypassStreamer) || + ROOT::Internal::MemberWiseElementsUseBytes(subinfo); + }); b >> dummy; // fLowerBound is ignored if (cl != subinfo->GetClass()) { Error("TClonesArray::Conversion to vector","Bad class"); diff --git a/io/io/src/TEmulatedCollectionProxy.cxx b/io/io/src/TEmulatedCollectionProxy.cxx index af15c32952a01..b041abdb4fe20 100644 --- a/io/io/src/TEmulatedCollectionProxy.cxx +++ b/io/io/src/TEmulatedCollectionProxy.cxx @@ -36,6 +36,7 @@ the class TEmulatedMapProxy. #include "TVirtualMutex.h" // For R__LOCKGUARD #include "TInterpreter.h" // For gInterpreterMutex #include "ROOT/CheckCollectionSize.hxx" +#include "ObjectWiseElementsUseBytes.h" // // Utility function to allow the creation of a TClass for a std::pair without @@ -666,7 +667,9 @@ void TEmulatedCollectionProxy::ReadBuffer(TBuffer &b, void *obj) TPushPop env(this,obj); int nElements = 0; b >> nElements; - nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedCollectionProxy::ReadBuffer", fName.c_str()); + nElements = + ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedCollectionProxy::ReadBuffer", fName.c_str(), + [&] { return ROOT::Internal::ObjectWiseElementsUseBytes(fKey, fVal); }); if ( fEnv->fObject ) { Resize(nElements,true); } @@ -682,7 +685,8 @@ void TEmulatedCollectionProxy::Streamer(TBuffer &b) int nElements = 0; b >> nElements; nElements = - ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedCollectionProxy::Streamer", fName.c_str()); + ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedCollectionProxy::Streamer", fName.c_str(), + [&] { return ROOT::Internal::ObjectWiseElementsUseBytes(fKey, fVal); }); if ( fEnv->fObject ) { Resize(nElements,true); } diff --git a/io/io/src/TEmulatedMapProxy.cxx b/io/io/src/TEmulatedMapProxy.cxx index c8f5de5aba1da..90eeb7658f748 100644 --- a/io/io/src/TEmulatedMapProxy.cxx +++ b/io/io/src/TEmulatedMapProxy.cxx @@ -36,6 +36,7 @@ is separated from the TEmulatedProxy. #include "TStreamerInfo.h" #include "TError.h" #include "ROOT/CheckCollectionSize.hxx" +#include "ObjectWiseElementsUseBytes.h" TEmulatedMapProxy::TEmulatedMapProxy(const TEmulatedMapProxy& copy) : TEmulatedCollectionProxy(copy) @@ -227,7 +228,9 @@ void TEmulatedMapProxy::ReadBuffer(TBuffer &b, void *obj) TPushPop env(this,obj); int nElements = 0; b >> nElements; - nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedMapProxy::ReadBuffer", fName.c_str()); + nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedMapProxy::ReadBuffer", fName.c_str(), [&] { + return ROOT::Internal::ObjectWiseElementsUseBytes(fKey, fVal); + }); if ( fEnv->fObject ) { Resize(nElements,true); } @@ -242,7 +245,9 @@ void TEmulatedMapProxy::Streamer(TBuffer &b) if ( b.IsReading() ) { //Read mode Int_t nElements = 0; b >> nElements; - nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedMapProxy::Streamer", fName.c_str()); + nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TEmulatedMapProxy::Streamer", fName.c_str(), [&] { + return ROOT::Internal::ObjectWiseElementsUseBytes(fKey, fVal); + }); if ( fEnv->fObject ) { Resize(nElements,true); } diff --git a/io/io/src/TGenCollectionStreamer.cxx b/io/io/src/TGenCollectionStreamer.cxx index a832a9fa568cc..eb1c990256847 100644 --- a/io/io/src/TGenCollectionStreamer.cxx +++ b/io/io/src/TGenCollectionStreamer.cxx @@ -30,6 +30,7 @@ size(), clear(), resize(). resize() may be a void operation. #include "TStreamerElement.h" #include "TVirtualCollectionIterators.h" #include "ROOT/CheckCollectionSize.hxx" +#include "ObjectWiseElementsUseBytes.h" #include @@ -1185,7 +1186,7 @@ void TGenCollectionStreamer::ReadBufferVectorPrimitives(TBuffer &b, void *obj, c int nElements = 0; b >> nElements; nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitives", - fName.c_str()); + fName.c_str(), [] { return true; }); fResize(obj,nElements); if (onFileClass) { @@ -1202,7 +1203,7 @@ void TGenCollectionStreamer::ReadBufferVectorPrimitivesFloat16(TBuffer &b, void int nElements = 0; b >> nElements; nElements = ROOT::Internal::CheckCollectionSize( - b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitivesFloat16", fName.c_str()); + b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitivesFloat16", fName.c_str(), [] { return true; }); fResize(obj,nElements); if (onFileClass) { @@ -1219,7 +1220,7 @@ void TGenCollectionStreamer::ReadBufferVectorPrimitivesDouble32(TBuffer &b, void int nElements = 0; b >> nElements; nElements = ROOT::Internal::CheckCollectionSize( - b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitivesDouble32", fName.c_str()); + b, nElements, "TGenCollectionStreamer::ReadBufferVectorPrimitivesDouble32", fName.c_str(), [] { return true; }); fResize(obj,nElements); if (onFileClass) { @@ -1326,7 +1327,8 @@ void TGenCollectionStreamer::ReadBufferGeneric(TBuffer &b, void *obj, const TCla int nElements = 0; b >> nElements; nElements = - ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::ReadBufferGeneric", fName.c_str()); + ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::ReadBufferGeneric", fName.c_str(), + [&] { return ROOT::Internal::ObjectWiseElementsUseBytes(fKey, fVal); }); if (nElements == 0) { if (obj) { @@ -1411,7 +1413,9 @@ void TGenCollectionStreamer::Streamer(TBuffer &b) if (b.IsReading()) { //Read mode int nElements = 0; b >> nElements; - nElements = ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::Streamer", fName.c_str()); + nElements = + ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::Streamer", fName.c_str(), + [&] { return ROOT::Internal::ObjectWiseElementsUseBytes(fKey, fVal); }); if (fEnv->fObject) { TGenCollectionProxy::Clear("force"); } @@ -1491,7 +1495,8 @@ void TGenCollectionStreamer::StreamerAsMap(TBuffer &b) int nElements = 0; b >> nElements; nElements = - ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::StreamerAsMap", fName.c_str()); + ROOT::Internal::CheckCollectionSize(b, nElements, "TGenCollectionStreamer::StreamerAsMap", fName.c_str(), + [&] { return ROOT::Internal::ObjectWiseElementsUseBytes(fKey, fVal); }); if (fEnv->fObject) { TGenCollectionProxy::Clear("force"); } diff --git a/io/io/src/TStreamerInfoActions.cxx b/io/io/src/TStreamerInfoActions.cxx index a2659caf88853..b16ed276aa253 100644 --- a/io/io/src/TStreamerInfoActions.cxx +++ b/io/io/src/TStreamerInfoActions.cxx @@ -786,7 +786,10 @@ namespace TStreamerInfoActions Int_t nobjects; buf.ReadInt(nobjects); nobjects = ROOT::Internal::CheckCollectionSize( - buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", config->fTypeName); + buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", config->fTypeName, [&] { + auto *memberWise = oldProxy->GetReadMemberWiseActions(vClVersion); + return memberWise && ROOT::Internal::MemberWiseElementsUseBytes(memberWise->fStreamerInfo); + }); void* alternative = oldProxy->Allocate(nobjects,true); if (nobjects) { TActionSequence *actions = oldProxy->GetReadMemberWiseActions( vClVersion ); @@ -820,7 +823,10 @@ namespace TStreamerInfoActions Int_t nobjects; buf.ReadInt(nobjects); nobjects = ROOT::Internal::CheckCollectionSize( - buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", config->fTypeName); + buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", config->fTypeName, [&] { + TClass *valueClass = oldProxy->GetValueClass(); + return valueClass && ROOT::Internal::MemberWiseElementsUseBytes(valueClass->GetStreamerInfo(0)); + }); void* env = oldProxy->Allocate(nobjects,true); if (nobjects || vers < 7 ) { @@ -862,7 +868,8 @@ namespace TStreamerInfoActions Int_t nobjects; buf.ReadInt(nobjects); nobjects = ROOT::Internal::CheckCollectionSize( - buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseSameClass", config->fTypeName); + buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseSameClass", config->fTypeName, + [&] { return actions && ROOT::Internal::MemberWiseElementsUseBytes(actions->fStreamerInfo); }); TVirtualCollectionProxy::TPushPop helper( oldProxy, (char*)obj ); void* alternative = oldProxy->Allocate(nobjects,true); if (nobjects) { @@ -901,7 +908,10 @@ namespace TStreamerInfoActions Int_t nobjects; buf.ReadInt(nobjects); nobjects = ROOT::Internal::CheckCollectionSize( - buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseSameClass", config->fTypeName); + buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseSameClass", config->fTypeName, [&] { + TClass *valueClass = oldProxy->GetValueClass(); + return valueClass && ROOT::Internal::MemberWiseElementsUseBytes(valueClass->GetStreamerInfo(0)); + }); void* env = oldProxy->Allocate(nobjects,true); if (nobjects || vers < 7 ) { @@ -940,7 +950,10 @@ namespace TStreamerInfoActions Int_t nobjects; buf.ReadInt(nobjects); nobjects = ROOT::Internal::CheckCollectionSize( - buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseChangedClass", config->fTypeName); + buf, nobjects, "TStreamerInfoActions::ReadSTLMemberWiseChangedClass", config->fTypeName, [&] { + auto *conversion = newProxy->GetConversionReadMemberWiseActions(oldProxy->GetValueClass(), vClVersion); + return conversion && ROOT::Internal::MemberWiseElementsUseBytes(conversion->fStreamerInfo); + }); void* alternative = newProxy->Allocate(nobjects,true); if (nobjects) { TActionSequence *actions = newProxy->GetConversionReadMemberWiseActions( oldProxy->GetValueClass(), vClVersion ); @@ -991,7 +1004,11 @@ namespace TStreamerInfoActions Int_t nobjects; buf.ReadInt(nobjects); nobjects = ROOT::Internal::CheckCollectionSize( - buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseChangedClass", config->fTypeName); + buf, nobjects, "TStreamerInfoActions::ReadArraySTLMemberWiseChangedClass", config->fTypeName, [&] { + auto *conversion = + newProxy->GetConversionReadMemberWiseActions(oldProxy->GetValueClass(), vClVersion); + return conversion && ROOT::Internal::MemberWiseElementsUseBytes(conversion->fStreamerInfo); + }); void* alternative = newProxy->Allocate(nobjects,true); if (nobjects) { TActionSequence *actions = newProxy->GetConversionReadMemberWiseActions( oldProxy->GetValueClass(), vClVersion ); @@ -2246,7 +2263,7 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadCollectionBasicType", - config->fTypeName); + config->fTypeName, [] { return true; }); vec->resize(nvalues); #ifdef R__VISUAL_CPLUSPLUS @@ -2293,7 +2310,7 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadCollectionFloat16", - config->fTypeName); + config->fTypeName, [] { return true; }); vec->resize(nvalues); #ifdef R__VISUAL_CPLUSPLUS @@ -2339,7 +2356,7 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadCollectionDouble32", - config->fTypeName); + config->fTypeName, [] { return true; }); vec->resize(nvalues); #ifdef R__VISUAL_CPLUSPLUS @@ -2386,8 +2403,9 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); - nvalues = ROOT::Internal::CheckCollectionSize( - buf, nvalues, "TStreamerInfoActions::ConvertCollectionBasicType", config->fTypeName); + nvalues = + ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ConvertCollectionBasicType", + config->fTypeName, [] { return true; }); vec->resize(nvalues); From *temp = new From[nvalues]; @@ -2415,8 +2433,9 @@ namespace TStreamerInfoActions std::vector *const vec = (std::vector*)(((char*)addr)+config->fOffset); Int_t nvalues; buf.ReadInt(nvalues); - nvalues = ROOT::Internal::CheckCollectionSize( - buf, nvalues, "TStreamerInfoActions::ConvertCollectionBasicType", config->fTypeName); + nvalues = + ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ConvertCollectionBasicType", + config->fTypeName, [] { return true; }); vec->resize(nvalues); From *temp = new From[nvalues]; @@ -2444,7 +2463,7 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ConvertCollectionDouble32", - config->fTypeName); + config->fTypeName, [] { return true; }); vec->resize(nvalues); Double32_t *temp = new Double32_t[nvalues]; @@ -2792,7 +2811,7 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadNumericalCollection", - config->fTypeName); + config->fTypeName, [] { return true; }); void* alternative = newProxy->Allocate(nvalues,true); if (nvalues) { char startbuf[TVirtualCollectionProxy::fgIteratorArenaSize]; @@ -3392,7 +3411,7 @@ namespace TStreamerInfoActions Int_t nvalues; buf.ReadInt(nvalues); nvalues = ROOT::Internal::CheckCollectionSize(buf, nvalues, "TStreamerInfoActions::ReadNumericalCollection", - config->fTypeName); + config->fTypeName, [] { return true; }); void* alternative = newProxy->Allocate(nvalues,true); if (nvalues) { char startbuf[TVirtualCollectionProxy::fgIteratorArenaSize]; diff --git a/io/io/src/TStreamerInfoReadBuffer.cxx b/io/io/src/TStreamerInfoReadBuffer.cxx index 0937717bf9372..b5664e8778e84 100644 --- a/io/io/src/TStreamerInfoReadBuffer.cxx +++ b/io/io/src/TStreamerInfoReadBuffer.cxx @@ -1194,8 +1194,9 @@ Int_t TStreamerInfo::ReadBuffer(TBuffer &b, const T &arr, TVirtualCollectionProxy::TPushPop helper( newProxy, cont ); Int_t nobjects; b >> nobjects; - nobjects = ROOT::Internal::CheckCollectionSize(b, nobjects, "TStreamerInfo::ReadBuffer", - cle->GetName()); + nobjects = ROOT::Internal::CheckCollectionSize( + b, nobjects, "TStreamerInfo::ReadBuffer", cle->GetName(), + [&] { return ROOT::Internal::MemberWiseElementsUseBytes(subinfo); }); env = newProxy->Allocate(nobjects,true); if (!nobjects && (vers>=7)) { // Do nothing but in version 6 of TStreamerInfo and below, @@ -1306,8 +1307,9 @@ Int_t TStreamerInfo::ReadBuffer(TBuffer &b, const T &arr, TVirtualCollectionProxy::TPushPop helper( newProxy, obj ); Int_t nobjects; b >> nobjects; - nobjects = ROOT::Internal::CheckCollectionSize(b, nobjects, "TStreamerInfo::ReadBuffer", - cle->GetName()); + nobjects = ROOT::Internal::CheckCollectionSize( + b, nobjects, "TStreamerInfo::ReadBuffer", cle->GetName(), + [&] { return ROOT::Internal::MemberWiseElementsUseBytes(subinfo); }); void* env = newProxy->Allocate(nobjects,true); if (!nobjects && (vers>=7)) { // Do nothing but in version 6 of TStreamerInfo and below, diff --git a/io/io/test/TBufferFileTests.cxx b/io/io/test/TBufferFileTests.cxx index c550e1fa410a3..141d2c0d7ce30 100644 --- a/io/io/test/TBufferFileTests.cxx +++ b/io/io/test/TBufferFileTests.cxx @@ -5,15 +5,18 @@ #include "TBufferFile.h" #include "TMemFile.h" #include "TClass.h" +#include "TClassStreamer.h" #include "TClonesArray.h" #include "TContainerConverters.h" #include "TInterpreter.h" #include "TKey.h" +#include "TMemberStreamer.h" #include "TNamed.h" #include "TVirtualCollectionProxy.h" #include #include #include +#include #include #include @@ -107,6 +110,12 @@ void CorruptInt(std::vector &data, unsigned int expected, unsigned int val data[pos + i] = char(value >> (8 * (3 - i))); } +/// Bytes left in `data` after the big-endian Int_t equal to `expected`. +unsigned int BytesAfterInt(const std::vector &data, unsigned int expected) +{ + return data.size() - std::min(data.size(), FindInt(data, expected) + 4); +} + std::vector Write(const void *obj, TClass *cl) { TBufferFile buf(TBuffer::kWrite); @@ -139,18 +148,124 @@ UInt_t CollectionSize(TClass *cl, void *obj) return cl->GetCollectionProxy()->Size(); } +/// Resize the collection of type `cl` at `obj` to `n` elements. +void ResizeCollection(TClass *cl, void *obj, UInt_t n) +{ + TVirtualCollectionProxy::TPushPop helper(cl->GetCollectionProxy(), obj); + cl->GetCollectionProxy()->Commit(cl->GetCollectionProxy()->Allocate(n, true)); +} + +/// Write a CorruptEvoA with `nFloats` floats and twice `nElems` elements that occupy no space to a file, and rename +/// its classes to CorruptEvoB and CorruptEvoBElem, so that reading it back goes through schema evolution. +std::vector WriteEvolvedFile(int nFloats, int nElems) +{ + TClass *cl = TClass::GetClass("CorruptEvoA"); + void *obj = cl->New(); + auto *floats = reinterpret_cast *>(static_cast(obj) + cl->GetDataMemberOffset("fFloats")); + floats->assign(nFloats, 1.5f); + ResizeCollection(TClass::GetClass("vector"), + static_cast(obj) + cl->GetDataMemberOffset("fElems"), nElems); + ResizeCollection(TClass::GetClass("vector"), + static_cast(obj) + cl->GetDataMemberOffset("fRenamed"), nElems); + std::vector data; + { + TMemFile file("evolution.root", "RECREATE", "", /*compress=*/0); + file.WriteObjectAny(obj, cl, "obj"); + file.Write(); + data.resize(file.GetSize()); + file.CopyTo(data.data(), data.size()); + } + cl->Destructor(obj); + const std::string from = "CorruptEvoA", to = "CorruptEvoB"; + for (auto it = data.begin(); (it = std::search(it, data.end(), from.begin(), from.end())) != data.end();) + it = std::copy(to.begin(), to.end(), it); + return data; +} + +void *ReadEvolvedFile(std::vector &data) +{ + TMemFile file("evolution.root", TMemFile::ZeroCopyView_t(data.data(), data.size())); + return file.GetObjectChecked("obj", "CorruptEvoB"); +} + class CorruptCollectionSize : public ::testing::Test { protected: static constexpr int kNObjs = 0x123; static constexpr int kNInts = 0x321; + static constexpr int kNFloats = 0x2345; std::vector fData; static void SetUpTestSuite() { + // Classes can only be declared and their streamers adopted once, e.g. with --gtest_repeat. + static bool done = false; + if (done) + return; + done = true; gInterpreter->Declare(TO_LITERAL(CORRUPTINNER)); gInterpreter->Declare(TO_LITERAL(CORRUPTOUTER)); gInterpreter->Declare(TO_LITERAL(CORRUPTNESTED)); + // Elements of a vector occupy no space on file. + gInterpreter->Declare(R"CODE( + struct CorruptNoBytes {}; + struct CorruptInPlace { + CorruptNoBytes *fObj = new CorruptNoBytes; //-> + }; + struct CorruptZeroByteHolder { + std::vector fInPlace; + std::vector fPtrs; + }; + // Version 1 is written as CorruptEvoA, and read back as CorruptEvoB after renaming it in the file. + struct CorruptEvoAElem { + CorruptNoBytes *fObj = new CorruptNoBytes; //-> + virtual ~CorruptEvoAElem() = default; + ClassDef(CorruptEvoAElem, 1); + }; + // The only data member of CorruptCustomMember is streamed by a custom member streamer that writes nothing. + struct CorruptCustomMember : TObject { + TObject *fObj = nullptr; + ClassDefOverride(CorruptCustomMember, 1); + }; + // CorruptOldElem is read as CorruptNewElem through a rename rule. + struct CorruptOldElem { + CorruptNoBytes *fObj = new CorruptNoBytes; //-> + virtual ~CorruptOldElem() = default; + ClassDef(CorruptOldElem, 1); + }; + struct CorruptNewElem { + CorruptNoBytes *fObj = new CorruptNoBytes; //-> + int fNew = 3; + virtual ~CorruptNewElem() = default; + ClassDef(CorruptNewElem, 1); + }; + struct CorruptEvoA { + std::vector fFloats; + std::vector fElems; + std::vector fRenamed; + virtual ~CorruptEvoA() = default; + ClassDef(CorruptEvoA, 1); + }; + struct CorruptEvoBElem { + CorruptNoBytes *fObj = new CorruptNoBytes; //-> + int fNew = 3; + virtual ~CorruptEvoBElem() = default; + ClassDef(CorruptEvoBElem, 2); + }; + struct CorruptEvoB { + std::vector fFloats; + std::vector fElems; + std::vector fRenamed; + virtual ~CorruptEvoB() = default; + ClassDef(CorruptEvoB, 2); + }; + )CODE"); + TClass::GetClass("CorruptNoBytes")->AdoptStreamer(new TClassStreamer([](TBuffer &, void *) {})); + TClass *customMember = TClass::GetClass("CorruptCustomMember"); + customMember->BuildRealData(); + customMember->AdoptMemberStreamer("*fObj", new TMemberStreamer([](TBuffer &, void *, Int_t) {})); + customMember->IgnoreTObjectStreamer(); + TClass::AddRule("CorruptNewElem version=[1-] sourceClass=CorruptOldElem targetClass=CorruptNewElem"); } void SetUp() override @@ -264,3 +379,214 @@ TEST_F(CorruptCollectionSize, ClonesArrayToProxy) EXPECT_EQ(CollectionSize(cl, read), 0u); cl->Destructor(read); } + +// A size larger than the bytes left in the buffer cannot be valid if each element occupies at least one byte. +TEST_F(CorruptCollectionSize, MemberWiseOversized) +{ + CorruptInt(fData, kNObjs, BytesAfterInt(fData, kNObjs) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TStreamerInfoActions::ReadSTLMemberWiseSameClass", "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto outer = Read(fData); + ASSERT_NE(outer, nullptr); + EXPECT_TRUE(outer->fObjs.empty()); + EXPECT_EQ(outer->fInts.size(), kNInts); +} + +TEST_F(CorruptCollectionSize, NestedOversized) +{ + CorruptNested nested; + nested.fOuters.resize(1); + nested.fOuters[0].fObjs.resize(kNObjs); + auto data = Write(nested); + CorruptInt(data, kNObjs, BytesAfterInt(data, kNObjs) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TStreamerInfo::ReadBuffer", "bytes are left in the buffer", /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto read = Read(data); + ASSERT_NE(read, nullptr); + ASSERT_EQ(read->fOuters.size(), 1u); + EXPECT_TRUE(read->fOuters[0].fObjs.empty()); +} + +TEST_F(CorruptCollectionSize, ClonesArrayToProxyOversized) +{ + TClass *cl = TClass::GetClass("vector"); + ASSERT_NE(cl, nullptr); + for (bool bypass : {true, false}) { + SCOPED_TRACE(bypass ? "member-wise" : "object-wise"); + TClonesArray clones("TNamed", kNObjs); + clones.BypassStreamer(bypass); + for (int i = 0; i < kNObjs; ++i) + clones.ConstructedAt(i); + TBufferFile wbuf(TBuffer::kWrite); + clones.Streamer(wbuf); + std::vector data(wbuf.Buffer(), wbuf.Buffer() + wbuf.Length()); + CorruptInt(data, kNObjs, BytesAfterInt(data, kNObjs) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TConvertClonesArrayToProxy::operator()", "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + TConvertClonesArrayToProxy convert(cl->GetCollectionProxy(), /*isPointer=*/false, /*isPrealloc=*/false); + void *read = cl->New(); + TBufferFile rbuf(TBuffer::kRead, data.size(), data.data(), /*adopt=*/false); + convert(rbuf, read); + EXPECT_EQ(CollectionSize(cl, read), 0u); + cl->Destructor(read); + } +} + +TEST_F(CorruptCollectionSize, BasicTypeOversized) +{ + CorruptInt(fData, kNInts, BytesAfterInt(fData, kNInts) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TStreamerInfoActions::ReadCollectionBasicType", "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto outer = Read(fData); + ASSERT_NE(outer, nullptr); + EXPECT_EQ(outer->fObjs.size(), kNObjs); + EXPECT_TRUE(outer->fInts.empty()); +} + +// A size equal to the bytes left in the buffer is not rejected. +TEST_F(CorruptCollectionSize, BasicTypeBoundary) +{ + const auto size = BytesAfterInt(fData, kNInts); + CorruptInt(fData, kNInts, size); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto outer = Read(fData); + ASSERT_NE(outer, nullptr); + EXPECT_EQ(outer->fInts.size(), size); +} + +TEST_F(CorruptCollectionSize, ObjectWiseOversized) +{ + std::vector strings(kNObjs, "a"); + auto data = Write(strings); + CorruptInt(data, kNObjs, BytesAfterInt(data, kNObjs) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TGenCollectionStreamer::ReadBufferGeneric", "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto read = Read>(data); + ASSERT_NE(read, nullptr); + EXPECT_TRUE(read->empty()); +} + +TEST_F(CorruptCollectionSize, EmulatedOversized) +{ + TClass *cl = TClass::GetClass("map"); + ASSERT_NE(cl, nullptr); + void *obj = cl->New(); + { + TVirtualCollectionProxy::TPushPop helper(cl->GetCollectionProxy(), obj); + cl->GetCollectionProxy()->Commit(cl->GetCollectionProxy()->Allocate(kNObjs, true)); + } + auto data = Write(obj, cl); + cl->Destructor(obj); + CorruptInt(data, kNObjs, BytesAfterInt(data, kNObjs) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TEmulatedMapProxy::ReadBuffer", "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + void *read = Read(data, cl); + ASSERT_NE(read, nullptr); + EXPECT_EQ(CollectionSize(cl, read), 0u); + cl->Destructor(read); +} + +// Valid collections whose elements occupy no space on file must still be read. +TEST_F(CorruptCollectionSize, ZeroByteElements) +{ + constexpr int kN = 1000; + TClass *cl = TClass::GetClass("CorruptZeroByteHolder"); + ASSERT_NE(cl, nullptr); + TClass *inPlace = TClass::GetClass("vector"); + TClass *ptrs = TClass::GetClass("vector"); + void *obj = cl->New(); + ResizeCollection(inPlace, static_cast(obj) + cl->GetDataMemberOffset("fInPlace"), kN); + ResizeCollection(ptrs, static_cast(obj) + cl->GetDataMemberOffset("fPtrs"), kN); + auto data = Write(obj, cl); + cl->Destructor(obj); + ASSERT_LT(data.size(), kN); + void *read = Read(data, cl); + ASSERT_NE(read, nullptr); + EXPECT_EQ(CollectionSize(inPlace, static_cast(read) + cl->GetDataMemberOffset("fInPlace")), kN); + EXPECT_EQ(CollectionSize(ptrs, static_cast(read) + cl->GetDataMemberOffset("fPtrs")), kN); + cl->Destructor(read); +} + +// The same holds for a TClonesArray read into a vector, here with objects whose only member has a custom streamer. +TEST_F(CorruptCollectionSize, ZeroByteClonesArrayToProxy) +{ + constexpr int kN = 1000; + TClonesArray clones("CorruptCustomMember", kN); + clones.ExpandCreate(kN); + TBufferFile wbuf(TBuffer::kWrite); + clones.Streamer(wbuf); + std::vector data(wbuf.Buffer(), wbuf.Buffer() + wbuf.Length()); + ASSERT_LT(data.size(), kN); + + TClass *cl = TClass::GetClass("vector"); + ASSERT_NE(cl, nullptr); + TConvertClonesArrayToProxy convert(cl->GetCollectionProxy(), /*isPointer=*/false, /*isPrealloc=*/false); + void *read = cl->New(); + TBufferFile rbuf(TBuffer::kRead, data.size(), data.data(), /*adopt=*/false); + convert(rbuf, read); + EXPECT_EQ(CollectionSize(cl, read), kN); + cl->Destructor(read); +} + +// The same holds when reading through schema evolution, which uses the layout on file: here from vector +// into vector, and elements that occupy no space on file but gained a data member, of the same class or of a +// class renamed by a rule. +TEST_F(CorruptCollectionSize, SchemaEvolution) +{ + constexpr int kNElems = 1000; + auto data = WriteEvolvedFile(kNFloats, kNElems); + TClass *cl = TClass::GetClass("CorruptEvoB"); + auto *read = static_cast(ReadEvolvedFile(data)); + ASSERT_NE(read, nullptr); + auto *doubles = reinterpret_cast *>(read + cl->GetDataMemberOffset("fFloats")); + ASSERT_EQ(doubles->size(), kNFloats); + EXPECT_EQ(doubles->front(), 1.5); + EXPECT_EQ(CollectionSize(TClass::GetClass("vector"), read + cl->GetDataMemberOffset("fElems")), + kNElems); + EXPECT_EQ(CollectionSize(TClass::GetClass("vector"), read + cl->GetDataMemberOffset("fRenamed")), + kNElems); + cl->Destructor(read); +} + +TEST_F(CorruptCollectionSize, SchemaEvolutionOversized) +{ + auto data = WriteEvolvedFile(kNFloats, 1); + // The rest of the file may contain the same 4 bytes, so look for the size followed by the first value, 1.5f. + const char pattern[8] = { + char(kNFloats >> 24), char(kNFloats >> 16), char(kNFloats >> 8), char(kNFloats), 0x3f, char(0xc0), 0, 0}; + const auto pos = std::search(data.begin(), data.end(), pattern, pattern + 8); + ASSERT_NE(pos, data.end()); + // One more than the bytes left after the size. + const unsigned int size = data.end() - (pos + 4) + 1; + for (int i = 0; i < 4; ++i) + pos[i] = char(size >> (8 * (3 - i))); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TStreamerInfoActions::ConvertCollectionBasicType", "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + TClass *cl = TClass::GetClass("CorruptEvoB"); + auto *read = static_cast(ReadEvolvedFile(data)); + ASSERT_NE(read, nullptr); + EXPECT_TRUE(reinterpret_cast *>(read + cl->GetDataMemberOffset("fFloats"))->empty()); + cl->Destructor(read); +} From 8cc6bfd7095c1ce6a722b2fffef0d4a08be9b02f Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Sat, 24 Oct 2026 11:20:36 +0200 Subject: [PATCH 3/5] [io] Validate string lengths when reading Strings of more than 254 characters store their length as an Int_t, which ReadStdString and ReadTString passed to std::string::resize and TString::Clobber unchecked. A corrupt length can thus throw std::length_error, or allocate up to 2 GB, before ReadFastArray finds that the buffer does not hold that many characters. ReadCharStar similarly allocated any positive length. Reject negative lengths, and lengths larger than the bytes left in the buffer, and read the string as empty. ReadCharStar keeps ignoring negative lengths silently, as before. Skipping a char* data member that was removed now goes through ReadCharStar too, rather than allocating the buffer itself. This reads the same bytes from a TBufferFile; TBufferJSON and TBufferXML now skip the member the way they wrote it. Assisted-by: claude-code:claude-opus-5-5 --- io/io/src/TBufferFile.cxx | 38 ++++++++++++- io/io/src/TStreamerInfoReadBuffer.cxx | 9 ++-- io/io/test/TBufferFileTests.cxx | 78 ++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 10 deletions(-) diff --git a/io/io/src/TBufferFile.cxx b/io/io/src/TBufferFile.cxx index a20a67ec60861..2657823f09bc7 100644 --- a/io/io/src/TBufferFile.cxx +++ b/io/io/src/TBufferFile.cxx @@ -180,6 +180,28 @@ void TBufferFile::ReadLong(Long_t &l) } } +//////////////////////////////////////////////////////////////////////////////// +/// Return whether the length of a string just read from `buf` can only come +/// from corrupt data, i.e. it is negative or larger than the bytes left in the +/// buffer, and report it from `method`. + +static bool IsCorruptStringLength(const TBufferFile &buf, Int_t length, const char *method) +{ + const Long64_t remaining = (Long64_t)buf.BufferSize() - buf.Length(); + if (R__likely(length >= 0 && length <= remaining)) + return false; + const char *parent = buf.GetParent() ? buf.GetParent()->GetName() : "memory/socket"; + if (length < 0) + buf.Error(method, "Corrupt data: the string read from %s has a negative length (%d); reading it as empty.", + parent, length); + else + buf.Error(method, + "Corrupt data: the string read from %s has a length of %d, but only %lld bytes are left in the " + "buffer; reading it as empty.", + parent, length, remaining); + return true; +} + //////////////////////////////////////////////////////////////////////////////// /// Read TString from TBuffer. @@ -192,10 +214,16 @@ void TBufferFile::ReadTString(TString &s) s.UnLink(); s.Zero(); } else { - if (nwh == 255) + if (nwh == 255) { *this >> nbig; - else + if (IsCorruptStringLength(*this, nbig, "ReadTString")) { + s.UnLink(); + s.Zero(); + return; + } + } else { nbig = nwh; + } nbig = s.Clobber(nbig); // update length since Clobber clamps to MaxSize (if Fatal does not abort) char *data = s.GetPointer(); @@ -245,6 +273,10 @@ void TBufferFile::ReadStdString(std::string *obj) } if (nwh == 255) { *this >> nbig; + if (IsCorruptStringLength(*this, nbig, "ReadStdString")) { + obj->clear(); + return; + } obj->resize(nbig,'\0'); ReadFastArray((char*)obj->data(),nbig); } @@ -294,6 +326,8 @@ void TBufferFile::ReadCharStar(char* &s) Error("ReadCharStar", "Cannot allocate buffer: maximum capacity exceeded (%d bytes)!", std::numeric_limits::max()); return; } + if (IsCorruptStringLength(*this, nch, "ReadCharStar")) + return; s = new char[nch+1]; ReadFastArray(s, nch); s[nch] = 0; diff --git a/io/io/src/TStreamerInfoReadBuffer.cxx b/io/io/src/TStreamerInfoReadBuffer.cxx index b5664e8778e84..8e9cdf9b6e619 100644 --- a/io/io/src/TStreamerInfoReadBuffer.cxx +++ b/io/io/src/TStreamerInfoReadBuffer.cxx @@ -259,12 +259,9 @@ Int_t TStreamerInfo::ReadBufferSkip(TBuffer &b, const T &arr, const TCompInfo *c // skip char* case TStreamerInfo::kSkip + TStreamerInfo::kCharStar: { DOLOOP { - Int_t nch; b >> nch; - if (nch>0) { - char* readbuf = new char[nch]; - b.ReadFastArray(readbuf,nch); - delete[] readbuf; - } + char *readbuf = nullptr; + b.ReadCharStar(readbuf); + delete[] readbuf; } break; } diff --git a/io/io/test/TBufferFileTests.cxx b/io/io/test/TBufferFileTests.cxx index 141d2c0d7ce30..0908ca560cd2c 100644 --- a/io/io/test/TBufferFileTests.cxx +++ b/io/io/test/TBufferFileTests.cxx @@ -12,6 +12,7 @@ #include "TKey.h" #include "TMemberStreamer.h" #include "TNamed.h" +#include "TString.h" #include "TVirtualCollectionProxy.h" #include #include @@ -155,12 +156,18 @@ void ResizeCollection(TClass *cl, void *obj, UInt_t n) cl->GetCollectionProxy()->Commit(cl->GetCollectionProxy()->Allocate(n, true)); } -/// Write a CorruptEvoA with `nFloats` floats and twice `nElems` elements that occupy no space to a file, and rename -/// its classes to CorruptEvoB and CorruptEvoBElem, so that reading it back goes through schema evolution. +constexpr int kTextLength = 0x135; + +/// Write a CorruptEvoA with a text of kTextLength characters, `nFloats` floats and twice `nElems` elements that occupy +/// no space to a file, and rename its classes to CorruptEvoB and CorruptEvoBElem, so that reading it back goes through +/// schema evolution. std::vector WriteEvolvedFile(int nFloats, int nElems) { TClass *cl = TClass::GetClass("CorruptEvoA"); void *obj = cl->New(); + std::string text(kTextLength, 'x'); + auto *textPtr = reinterpret_cast(static_cast(obj) + cl->GetDataMemberOffset("fText")); + *textPtr = text.data(); auto *floats = reinterpret_cast *>(static_cast(obj) + cl->GetDataMemberOffset("fFloats")); floats->assign(nFloats, 1.5f); ResizeCollection(TClass::GetClass("vector"), @@ -175,6 +182,7 @@ std::vector WriteEvolvedFile(int nFloats, int nElems) data.resize(file.GetSize()); file.CopyTo(data.data(), data.size()); } + *textPtr = nullptr; cl->Destructor(obj); const std::string from = "CorruptEvoA", to = "CorruptEvoB"; for (auto it = data.begin(); (it = std::search(it, data.end(), from.begin(), from.end())) != data.end();) @@ -243,6 +251,7 @@ class CorruptCollectionSize : public ::testing::Test { std::vector fFloats; std::vector fElems; std::vector fRenamed; + char *fText = nullptr; // Removed in version 2 virtual ~CorruptEvoA() = default; ClassDef(CorruptEvoA, 1); }; @@ -555,6 +564,7 @@ TEST_F(CorruptCollectionSize, SchemaEvolution) constexpr int kNElems = 1000; auto data = WriteEvolvedFile(kNFloats, kNElems); TClass *cl = TClass::GetClass("CorruptEvoB"); + ROOT::TestSupport::CheckDiagsRAII diags; auto *read = static_cast(ReadEvolvedFile(data)); ASSERT_NE(read, nullptr); auto *doubles = reinterpret_cast *>(read + cl->GetDataMemberOffset("fFloats")); @@ -590,3 +600,67 @@ TEST_F(CorruptCollectionSize, SchemaEvolutionOversized) EXPECT_TRUE(reinterpret_cast *>(read + cl->GetDataMemberOffset("fFloats"))->empty()); cl->Destructor(read); } + +// Strings longer than 254 characters store their length as an Int_t. +TEST_F(CorruptCollectionSize, String) +{ + constexpr unsigned int kLength = 0x1234; + const auto checkCorruptLength = [](auto str, const char *location, bool negative) { + auto data = Write(str); + CorruptInt(data, kLength, negative ? kCorruptSize : BytesAfterInt(data, kLength) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, location, negative ? "negative length" : "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto read = Read(data); + ASSERT_NE(read, nullptr); + EXPECT_TRUE(*read == decltype(str)()); + }; + for (bool negative : {true, false}) { + SCOPED_TRACE(negative ? "negative" : "oversized"); + checkCorruptLength(std::string(kLength, 'a'), "TBufferFile::ReadStdString", negative); + checkCorruptLength(TString('a', kLength), "TBufferFile::ReadTString", negative); + } +} + +TEST_F(CorruptCollectionSize, CharStar) +{ + constexpr unsigned int kLength = 0x1234; + const std::string str(kLength, 'a'); + TBufferFile wbuf(TBuffer::kWrite); + wbuf.WriteCharStar(const_cast(str.c_str())); + std::vector data(wbuf.Buffer(), wbuf.Buffer() + wbuf.Length()); + CorruptInt(data, kLength, BytesAfterInt(data, kLength) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TBufferFile::ReadCharStar", "bytes are left in the buffer", /*matchFullMessage=*/false); + TBufferFile rbuf(TBuffer::kRead, data.size(), data.data(), /*adopt=*/false); + char *read = nullptr; + rbuf.ReadCharStar(read); + EXPECT_EQ(read, nullptr); + delete[] read; +} + +// A char* data member that was removed is skipped with ReadCharStar. +TEST_F(CorruptCollectionSize, SkippedCharStar) +{ + auto data = WriteEvolvedFile(kNFloats, 1); + // The rest of the file may contain the same 4 bytes, so look for the length followed by the first characters. + const char pattern[8] = { + char(kTextLength >> 24), char(kTextLength >> 16), char(kTextLength >> 8), char(kTextLength), 'x', 'x', 'x', 'x'}; + const auto pos = std::search(data.begin(), data.end(), pattern, pattern + 8); + ASSERT_NE(pos, data.end()); + // One more than the bytes left after the length. + const unsigned int size = data.end() - (pos + 4) + 1; + for (int i = 0; i < 4; ++i) + pos[i] = char(size >> (8 * (3 - i))); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TBufferFile::ReadCharStar", "bytes are left in the buffer", /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + TClass *cl = TClass::GetClass("CorruptEvoB"); + auto *read = static_cast(ReadEvolvedFile(data)); + ASSERT_NE(read, nullptr); + cl->Destructor(read); +} From 3bec77bda4fd73318fc1cc50daeb4cc8b9a4f51b Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Sat, 24 Oct 2026 11:24:20 +0200 Subject: [PATCH 4/5] [cont] Bound object counts by the remaining bytes TObjArray, TRefArray and TClonesArray read their number of objects as an Int_t and expand their storage to that size before reading the objects. A corrupt count can thus allocate up to 2^31 slots, and for TClonesArray as many objects, before reading them past the end of the buffer. TClonesArray also takes the absolute value of negative counts, which is undefined for INT_MIN and left it negative. Reject negative counts, as for STL collections. Each object of a TObjArray and TRefArray occupies at least 4 bytes on file, and each object of a TClonesArray one byte unless the streamer is bypassed, in which case it depends on the layout of the class. When that holds, a count larger than the bytes left in the buffer can only come from corrupt data, so report it and read the collection as empty too. Assisted-by: claude-code:claude-opus-5-5 --- core/cont/src/TClonesArray.cxx | 11 +- core/cont/src/TObjArray.cxx | 4 + core/cont/src/TRefArray.cxx | 4 + io/io/test/CMakeLists.txt | 1 + io/io/test/CorruptObjectCount.cxx | 173 ++++++++++++++++++++++++++++++ 5 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 io/io/test/CorruptObjectCount.cxx diff --git a/core/cont/src/TClonesArray.cxx b/core/cont/src/TClonesArray.cxx index 6d464ffa81aec..c615666d40e90 100644 --- a/core/cont/src/TClonesArray.cxx +++ b/core/cont/src/TClonesArray.cxx @@ -131,8 +131,10 @@ When investigating misuse of TClonesArray, please make sure of the following: #include "TClass.h" #include "TObject.h" #include "TObjectTable.h" +#include "ROOT/CheckCollectionSize.hxx" #include +#include #include @@ -779,9 +781,16 @@ void TClonesArray::Streamer(TBuffer &b) } b >> nobjects; - if (nobjects < 0) + if (nobjects < 0 && nobjects != std::numeric_limits::min()) nobjects = -nobjects; // still there for backward compatibility b >> fLowerBound; + // Without bypassing the streamer, each object starts with a byte telling whether it is present. Otherwise, the + // objects are read below with the layout of fClass, which stays the target class if the user changed it. + nobjects = ROOT::Internal::CheckCollectionSize(b, nobjects, "TClonesArray::Streamer", "TClonesArray", [&] { + TClass *readClass = fClass && classv != fClass->GetName() ? fClass : cl; + return !CanBypassStreamer() || b.TestBit(TBuffer::kCannotHandleMemberWiseStreaming) || + ROOT::Internal::MemberWiseElementsUseBytes(readClass->GetStreamerInfo(clv)); + }); if (!fClass) { fClass = cl; if (!fKeep) { diff --git a/core/cont/src/TObjArray.cxx b/core/cont/src/TObjArray.cxx index 804de770168e9..b5d3b2d299c12 100644 --- a/core/cont/src/TObjArray.cxx +++ b/core/cont/src/TObjArray.cxx @@ -52,6 +52,7 @@ set to the owner of its own content. #include "TROOT.h" #include "TBuffer.h" #include "TVirtualMutex.h" +#include "ROOT/CheckCollectionSize.hxx" #include @@ -460,6 +461,9 @@ void TObjArray::Streamer(TBuffer &b) b >> nobjects; b >> fLowerBound; + // Each object occupies at least one byte on file, as it is stored as a pointer, i.e. a tag of 4 bytes. + nobjects = + ROOT::Internal::CheckCollectionSize(b, nobjects, "TObjArray::Streamer", "TObjArray", [] { return true; }); if (nobjects >= fSize) Expand(nobjects); fLast = -1; TObject *obj; diff --git a/core/cont/src/TRefArray.cxx b/core/cont/src/TRefArray.cxx index 8c6c56e16d3dc..f5b1d461d1254 100644 --- a/core/cont/src/TRefArray.cxx +++ b/core/cont/src/TRefArray.cxx @@ -99,6 +99,7 @@ Fatal in : Maximum number of TProcessID (65535) has be #include "TRefTable.h" #include "TBuffer.h" #include "TError.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBits.h" @@ -524,6 +525,9 @@ void TRefArray::Streamer(TBuffer &R__b) fName.Streamer(R__b); R__b >> nobjects; R__b >> fLowerBound; + // Each object occupies at least one byte on file, as it is stored as its unique ID of 4 bytes. + nobjects = + ROOT::Internal::CheckCollectionSize(R__b, nobjects, "TRefArray::Streamer", "TRefArray", [] { return true; }); if (nobjects >= fSize) Expand(nobjects); fLast = -1; R__b >> pidf; diff --git a/io/io/test/CMakeLists.txt b/io/io/test/CMakeLists.txt index ebb379cf2955d..49c7dfe265fb1 100644 --- a/io/io/test/CMakeLists.txt +++ b/io/io/test/CMakeLists.txt @@ -7,6 +7,7 @@ ROOT_ADD_GTEST(RRawFile RRawFile.cxx LIBRARIES RIO) ROOT_ADD_GTEST(TFile TFileTests.cxx LIBRARIES RIO) ROOT_ADD_GTEST(TBufferFile TBufferFileTests.cxx LIBRARIES RIO) +ROOT_ADD_GTEST(CorruptObjectCount CorruptObjectCount.cxx LIBRARIES RIO) ROOT_ADD_GTEST(TBufferMerger TBufferMerger.cxx LIBRARIES RIO Imt Tree) ROOT_ADD_GTEST(TBufferJSON TBufferJSONTests.cxx LIBRARIES RIO) ROOT_ADD_GTEST(TFileMerger TFileMergerTests.cxx LIBRARIES RIO Tree Hist) diff --git a/io/io/test/CorruptObjectCount.cxx b/io/io/test/CorruptObjectCount.cxx new file mode 100644 index 0000000000000..db82af80b4979 --- /dev/null +++ b/io/io/test/CorruptObjectCount.cxx @@ -0,0 +1,173 @@ +#include "gtest/gtest.h" + +#include "ROOT/TestSupport.hxx" + +#include "TBufferFile.h" +#include "TClass.h" +#include "TClonesArray.h" +#include "TInterpreter.h" +#include "TNamed.h" +#include "TObjArray.h" +#include "TRefArray.h" + +#include +#include +#include +#include +#include + +// A number of objects read from corrupt data must not be used to allocate memory. + +namespace { + +constexpr int kN = 0x123; + +/// Position of the big-endian Int_t equal to `expected` in `data`, or data.end(). Unless `firstOccurrence`, it must +/// occur exactly once. +std::vector::iterator FindInt(std::vector &data, unsigned int expected, bool firstOccurrence) +{ + const char pattern[4] = {char(expected >> 24), char(expected >> 16), char(expected >> 8), char(expected)}; + const auto it = std::search(data.begin(), data.end(), pattern, pattern + 4); + if (it == data.end() || (!firstOccurrence && std::search(it + 1, data.end(), pattern, pattern + 4) != data.end())) { + ADD_FAILURE() << "expected exactly one occurrence of " << expected << " in the serialized data"; + return data.end(); + } + return it; +} + +/// Overwrite the big-endian Int_t at `it` with `value`. +void SetInt(std::vector::iterator it, unsigned int value) +{ + for (int i = 0; i < 4; ++i) + *(it + i) = char(value >> (8 * (3 - i))); +} + +template +std::vector Write(T &obj) +{ + TBufferFile buf(TBuffer::kWrite); + buf.WriteObject(&obj); + return std::vector(buf.Buffer(), buf.Buffer() + buf.Length()); +} + +template +std::unique_ptr Read(std::vector &data) +{ + TBufferFile buf(TBuffer::kRead, data.size(), data.data(), /*adopt=*/false); + return std::unique_ptr(static_cast(buf.ReadObject(T::Class()))); +} + +template +void CheckOversized(T &array, bool firstOccurrence = false) +{ + auto data = Write(array); + const auto it = FindInt(data, kN, firstOccurrence); + ASSERT_NE(it, data.end()); + // The smallest number of objects that cannot fit in the rest of the buffer. Without the check, reading them runs + // past the end of the buffer rather than allocating gigabytes. + SetInt(it, data.end() - (it + 4) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, std::string(T::Class_Name()) + "::Streamer", "bytes are left in the buffer", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto read = Read(data); + ASSERT_NE(read, nullptr); + EXPECT_EQ(read->GetEntriesFast(), 0); +} + +template +void CheckNegative(T &array, bool firstOccurrence = false) +{ + auto data = Write(array); + const auto it = FindInt(data, kN, firstOccurrence); + ASSERT_NE(it, data.end()); + SetInt(it, std::numeric_limits::min()); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, std::string(T::Class_Name()) + "::Streamer", "negative number of elements", + /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto read = Read(data); + ASSERT_NE(read, nullptr); + EXPECT_EQ(read->GetEntriesFast(), 0); +} + +} // namespace + +TEST(CorruptObjectCount, TObjArray) +{ + TObjArray array; + array.SetOwner(); + for (int i = 0; i < kN; ++i) + array.Add(new TNamed("a", "")); + CheckOversized(array); + CheckNegative(array); +} + +TEST(CorruptObjectCount, TRefArray) +{ + std::vector objects(kN); + TRefArray array; + for (auto &obj : objects) + array.Add(&obj); + // The unique IDs of the objects come after their number and may include it. + CheckOversized(array, /*firstOccurrence=*/true); + CheckNegative(array, /*firstOccurrence=*/true); +} + +TEST(CorruptObjectCount, TClonesArray) +{ + for (bool bypass : {true, false}) { + SCOPED_TRACE(bypass ? "bypass streamer" : "object-wise"); + TClonesArray array("TNamed"); + array.BypassStreamer(bypass); + for (int i = 0; i < kN; ++i) + new (array[i]) TNamed("a", ""); + CheckOversized(array); + } +} + +// TClonesArray takes the absolute value of negative numbers of objects, which is undefined for INT_MIN. +TEST(CorruptObjectCount, TClonesArrayMinInt) +{ + TClonesArray array("TNamed"); + for (int i = 0; i < kN; ++i) + new (array[i]) TNamed("a", ""); + auto data = Write(array); + const auto it = FindInt(data, kN, /*firstOccurrence=*/false); + ASSERT_NE(it, data.end()); + SetInt(it, std::numeric_limits::min()); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TClonesArray::Streamer", "negative number of elements", /*matchFullMessage=*/false); + diags.optionalDiag(kError, "TBufferFile::CheckByteCount", "read too few bytes", /*matchFullMessage=*/false); + auto read = Read(data); + ASSERT_NE(read, nullptr); + EXPECT_EQ(read->GetEntriesFast(), 0); +} + +// Valid TClonesArrays whose objects occupy no space on file must still be read. +TEST(CorruptObjectCount, TClonesArrayZeroByteObjects) +{ + // The class can only be declared once, e.g. with --gtest_repeat. + static const bool declared = gInterpreter->Declare(R"CODE( + struct CorruptEmptyObject : TObject { + ClassDefOverride(CorruptEmptyObject, 1); + }; + )CODE"); + ASSERT_TRUE(declared); + TClass *cl = TClass::GetClass("CorruptEmptyObject"); + ASSERT_NE(cl, nullptr); + cl->IgnoreTObjectStreamer(); + + constexpr int kMany = 1000; + TClonesArray array("CorruptEmptyObject"); + array.ExpandCreate(kMany); + auto data = Write(array); + ASSERT_LT(data.size(), kMany); + + auto read = Read(data); + ASSERT_NE(read, nullptr); + EXPECT_EQ(read->GetEntriesFast(), kMany); +} From f9dfd35802cbc5b514c5e45df03c3583e66c1944 Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Sat, 24 Oct 2026 11:28:18 +0200 Subject: [PATCH 5/5] [cont] Bound TArray sizes by the remaining bytes The TArray streamers read their size as an Int_t and resize the array to it before ReadFastArray checks that the buffer holds that many elements, so a corrupt size can allocate up to 16 GB for a TArrayD. These arrays hold e.g. the bin contents of histograms. Each element occupies at least one byte on file, so reject sizes that are negative or larger than the bytes left in the buffer, and read the array as empty, as for the other collections. Assisted-by: claude-code:claude-opus-5-5 --- core/cont/src/TArrayC.cxx | 3 +++ core/cont/src/TArrayD.cxx | 3 +++ core/cont/src/TArrayF.cxx | 3 +++ core/cont/src/TArrayI.cxx | 3 +++ core/cont/src/TArrayL.cxx | 3 +++ core/cont/src/TArrayL64.cxx | 3 +++ core/cont/src/TArrayS.cxx | 3 +++ io/io/test/CorruptObjectCount.cxx | 41 +++++++++++++++++++++++++++++++ 8 files changed, 62 insertions(+) diff --git a/core/cont/src/TArrayC.cxx b/core/cont/src/TArrayC.cxx index c54f6d6399276..88fd28a568194 100644 --- a/core/cont/src/TArrayC.cxx +++ b/core/cont/src/TArrayC.cxx @@ -15,6 +15,7 @@ Array of chars or bytes (8 bits per element). */ #include "TArrayC.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBuffer.h" @@ -149,6 +150,8 @@ void TArrayC::Streamer(TBuffer &b) if (b.IsReading()) { Int_t n; b >> n; + // Each element occupies at least one byte on file. + n = ROOT::Internal::CheckCollectionSize(b, n, "TArrayC::Streamer", "TArrayC", [] { return true; }); Set(n); b.ReadFastArray(fArray,n); } else { diff --git a/core/cont/src/TArrayD.cxx b/core/cont/src/TArrayD.cxx index bea84d4ac5eaa..9815987aec6fd 100644 --- a/core/cont/src/TArrayD.cxx +++ b/core/cont/src/TArrayD.cxx @@ -15,6 +15,7 @@ Array of doubles (64 bits per element). */ #include "TArrayD.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBuffer.h" @@ -150,6 +151,8 @@ void TArrayD::Streamer(TBuffer &b) if (b.IsReading()) { Int_t n; b >> n; + // Each element occupies at least one byte on file. + n = ROOT::Internal::CheckCollectionSize(b, n, "TArrayD::Streamer", "TArrayD", [] { return true; }); Set(n); b.ReadFastArray(fArray,n); } else { diff --git a/core/cont/src/TArrayF.cxx b/core/cont/src/TArrayF.cxx index 59d76a9488143..e0f60b1848b5f 100644 --- a/core/cont/src/TArrayF.cxx +++ b/core/cont/src/TArrayF.cxx @@ -15,6 +15,7 @@ Array of floats (32 bits per element). */ #include "TArrayF.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBuffer.h" @@ -149,6 +150,8 @@ void TArrayF::Streamer(TBuffer &b) if (b.IsReading()) { Int_t n; b >> n; + // Each element occupies at least one byte on file. + n = ROOT::Internal::CheckCollectionSize(b, n, "TArrayF::Streamer", "TArrayF", [] { return true; }); Set(n); b.ReadFastArray(fArray,n); } else { diff --git a/core/cont/src/TArrayI.cxx b/core/cont/src/TArrayI.cxx index f17c8b74be16a..7c4b658a8f136 100644 --- a/core/cont/src/TArrayI.cxx +++ b/core/cont/src/TArrayI.cxx @@ -15,6 +15,7 @@ Array of integers (32 bits per element). */ #include "TArrayI.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBuffer.h" @@ -149,6 +150,8 @@ void TArrayI::Streamer(TBuffer &b) if (b.IsReading()) { Int_t n; b >> n; + // Each element occupies at least one byte on file. + n = ROOT::Internal::CheckCollectionSize(b, n, "TArrayI::Streamer", "TArrayI", [] { return true; }); Set(n); b.ReadFastArray(fArray,n); } else { diff --git a/core/cont/src/TArrayL.cxx b/core/cont/src/TArrayL.cxx index a4480ad4b7bfe..e20ad4f067b83 100644 --- a/core/cont/src/TArrayL.cxx +++ b/core/cont/src/TArrayL.cxx @@ -15,6 +15,7 @@ Array of longs (32 or 64 bits per element). */ #include "TArrayL.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBuffer.h" @@ -149,6 +150,8 @@ void TArrayL::Streamer(TBuffer &b) if (b.IsReading()) { Int_t n; b >> n; + // Each element occupies at least one byte on file. + n = ROOT::Internal::CheckCollectionSize(b, n, "TArrayL::Streamer", "TArrayL", [] { return true; }); Set(n); b.ReadFastArray(fArray,n); } else { diff --git a/core/cont/src/TArrayL64.cxx b/core/cont/src/TArrayL64.cxx index 3ae8427811ce9..415636a1f9106 100644 --- a/core/cont/src/TArrayL64.cxx +++ b/core/cont/src/TArrayL64.cxx @@ -15,6 +15,7 @@ Array of long64s (64 bits per element). */ #include "TArrayL64.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBuffer.h" @@ -149,6 +150,8 @@ void TArrayL64::Streamer(TBuffer &b) if (b.IsReading()) { Int_t n; b >> n; + // Each element occupies at least one byte on file. + n = ROOT::Internal::CheckCollectionSize(b, n, "TArrayL64::Streamer", "TArrayL64", [] { return true; }); Set(n); b.ReadFastArray(fArray,n); } else { diff --git a/core/cont/src/TArrayS.cxx b/core/cont/src/TArrayS.cxx index a00dad0146c9a..25595bba73805 100644 --- a/core/cont/src/TArrayS.cxx +++ b/core/cont/src/TArrayS.cxx @@ -15,6 +15,7 @@ Array of shorts (16 bits per element). */ #include "TArrayS.h" +#include "ROOT/CheckCollectionSize.hxx" #include "TBuffer.h" @@ -149,6 +150,8 @@ void TArrayS::Streamer(TBuffer &b) if (b.IsReading()) { Int_t n; b >> n; + // Each element occupies at least one byte on file. + n = ROOT::Internal::CheckCollectionSize(b, n, "TArrayS::Streamer", "TArrayS", [] { return true; }); Set(n); b.ReadFastArray(fArray,n); } else { diff --git a/io/io/test/CorruptObjectCount.cxx b/io/io/test/CorruptObjectCount.cxx index db82af80b4979..f6b5e57e57857 100644 --- a/io/io/test/CorruptObjectCount.cxx +++ b/io/io/test/CorruptObjectCount.cxx @@ -2,6 +2,13 @@ #include "ROOT/TestSupport.hxx" +#include "TArrayC.h" +#include "TArrayD.h" +#include "TArrayF.h" +#include "TArrayI.h" +#include "TArrayL.h" +#include "TArrayL64.h" +#include "TArrayS.h" #include "TBufferFile.h" #include "TClass.h" #include "TClonesArray.h" @@ -93,6 +100,29 @@ void CheckNegative(T &array, bool firstOccurrence = false) EXPECT_EQ(read->GetEntriesFast(), 0); } +template +void CheckArray(const char *location) +{ + for (bool negative : {true, false}) { + SCOPED_TRACE(negative ? "negative" : "oversized"); + T array(kN); + TBufferFile wbuf(TBuffer::kWrite); + array.Streamer(wbuf); + std::vector data(wbuf.Buffer(), wbuf.Buffer() + wbuf.Length()); + const auto it = FindInt(data, kN, /*firstOccurrence=*/false); + ASSERT_NE(it, data.end()); + SetInt(it, negative ? std::numeric_limits::min() : data.end() - (it + 4) + 1); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, location, negative ? "negative number of elements" : "bytes are left in the buffer", + /*matchFullMessage=*/false); + T read; + TBufferFile rbuf(TBuffer::kRead, data.size(), data.data(), /*adopt=*/false); + read.Streamer(rbuf); + EXPECT_EQ(read.GetSize(), 0); + } +} + } // namespace TEST(CorruptObjectCount, TObjArray) @@ -171,3 +201,14 @@ TEST(CorruptObjectCount, TClonesArrayZeroByteObjects) ASSERT_NE(read, nullptr); EXPECT_EQ(read->GetEntriesFast(), kMany); } + +TEST(CorruptObjectCount, TArray) +{ + CheckArray("TArrayC::Streamer"); + CheckArray("TArrayS::Streamer"); + CheckArray("TArrayI::Streamer"); + CheckArray("TArrayL::Streamer"); + CheckArray("TArrayL64::Streamer"); + CheckArray("TArrayF::Streamer"); + CheckArray("TArrayD::Streamer"); +}