diff --git a/io/io/src/TBufferFile.cxx b/io/io/src/TBufferFile.cxx index dd71715942c1d..d0e0047612ab5 100644 --- a/io/io/src/TBufferFile.cxx +++ b/io/io/src/TBufferFile.cxx @@ -440,7 +440,10 @@ Long64_t TBufferFile::CheckByteCount(ULong64_t startpos, ULong64_t bcnt, const T // The position is above 4GB but was cached using a 32 bit variable. startpos = fByteCountStack.back().locator; // See below - R__ASSERT((fByteCountStack.back().cl == nullptr || clss == fByteCountStack.back().cl) + R__ASSERT((fByteCountStack.back().cl == nullptr || + (clss == nullptr && (classname == nullptr || classname[0] == '\0')) || + (clss && clss == fByteCountStack.back().cl) || + (classname && strcmp(classname, fByteCountStack.back().cl->GetName()) == 0)) && "Class on the byte count position stack does not match the passed class"); } else { // This assert allows to reject cases that used to be valid (missing or @@ -2658,6 +2661,8 @@ void TBufferFile::SkipObjectAny(Long64_t start, UInt_t count) /// real beginning of the object in memory. You will need to use a /// dynamic_cast later if you need to retrieve it. + + void *TBufferFile::ReadObjectAny(const TClass *clCast) { R__ASSERT(IsReading()); @@ -2665,6 +2670,20 @@ void *TBufferFile::ReadObjectAny(const TClass *clCast) // make sure fMap is initialized InitMap(); + struct CaptureAndCheck { + using ByteCountStack_t = TBufferFile::ByteCountStack_t; + size_t fCurrent; + ByteCountStack_t *fByteCountStack; + + CaptureAndCheck(ByteCountStack_t *stack) : fCurrent(stack->size()), fByteCountStack(stack) {} + ~CaptureAndCheck() { + if (fCurrent != fByteCountStack->size()) { + ::Fatal("CaptureAndCheck", "Byte count stack was not properly cleaned up. Current size is %zu but should be %zu", fByteCountStack->size(), fCurrent); + } + } + }; + CaptureAndCheck checker(&(this->fByteCountStack)); + // before reading object save start position ULong64_t startpos = static_cast(fBufCur-fBuffer); ULong64_t cntpos = startpos <= kMaxCountPosition ? startpos : kOverflowPosition; @@ -3015,6 +3034,14 @@ TClass *TBufferFile::ReadClass(const TClass *clReq, ULong64_t *objTag) // return bytecount in objTag if (objTag) { *objTag = (bcnt & ~kByteCountMask); + if (*objTag == 0) { + // The byte count was stored but is zero, this means the data + // did not fit and thus we stored it in 'fByteCounts' instead. + // Mark this case by setting startpos to kOverflowCount. + *objTag = kOverflowCount; + // We do not have access to the caller's "cntpos" and can not + // update to kOverflowPosition (unlike the same code in ReadVersion). + } if (cl) fByteCountStack.back().alt = cl; } @@ -3159,7 +3186,8 @@ Version_t TBufferFile::ReadVersion(UInt_t *startpos, UInt_t *bcnt, const TClass // before reading object save start position auto full_startpos = fBufCur - fBuffer; *startpos = full_startpos <= kMaxCountPosition ? UInt_t(full_startpos) : kOverflowPosition; - fByteCountStack.push_back({(size_t)full_startpos, cl, nullptr}); + if (bcnt) + fByteCountStack.push_back({(size_t)full_startpos, cl, nullptr}); } // read byte count (older files don't have byte count) @@ -3196,7 +3224,8 @@ Version_t TBufferFile::ReadVersion(UInt_t *startpos, UInt_t *bcnt, const TClass // did not fit and thus we stored it in 'fByteCounts' instead. // Mark this case by setting startpos to kOverflowCount. *bcnt = kOverflowCount; - *startpos = kOverflowPosition; + if (startpos) + *startpos = kOverflowPosition; } } } @@ -3302,7 +3331,8 @@ Version_t TBufferFile::ReadVersionNoCheckSum(UInt_t *startpos, UInt_t *bcnt) auto full_startpos = fBufCur - fBuffer; *startpos = full_startpos < kMaxCountPosition ? UInt_t(full_startpos) : kOverflowPosition; // TODO: Extend ReadVersionNoCheckSum to take the class pointer. - fByteCountStack.push_back({(size_t)full_startpos, nullptr, nullptr}); + if (bcnt) + fByteCountStack.push_back({(size_t)full_startpos, nullptr, nullptr}); } // read byte count (older files don't have byte count) @@ -3338,7 +3368,8 @@ Version_t TBufferFile::ReadVersionNoCheckSum(UInt_t *startpos, UInt_t *bcnt) // did not fit and thus we stored it in 'fByteCounts' instead. // Mark this case by setting startpos to kOverflowCount. *bcnt = kOverflowCount; - *startpos = kOverflowPosition; + if (startpos) + *startpos = kOverflowPosition; } } } @@ -3847,6 +3878,7 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, const TClass else version = ReadVersion(&R__s, &R__c, cl); + size_t current = fByteCountStack.size(); Bool_t v2file = kFALSE; TFile *file = (TFile*)GetParent(); if (file && file->GetVersion() < 30000) { @@ -3865,6 +3897,7 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, const TClass Error("ReadClassBuffer", "Could not find the right streamer info to convert %s version %d into a %s, object skipped at offset %d", onFileClass->GetName(), version, cl->GetName(), Length() ); + --current; CheckByteCount(R__s, R__c, onFileClass); return 0; } @@ -3889,6 +3922,7 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, const TClass if (version < -1 || version >= infocapacity) { Error("ReadClassBuffer","class: %s, attempting to access a wrong version: %d, object skipped at offset %d", cl->GetName(), version, Length()); + --current; CheckByteCount(R__s, R__c, cl); return 0; } @@ -3947,11 +3981,13 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, const TClass // When the object was written the class was version zero, so // there is no StreamerInfo to be found. // Check that the buffer position corresponds to the byte count. + --current; CheckByteCount(R__s, R__c, cl); return 0; } else { Error( "ReadClassBuffer", "Could not find the StreamerInfo for version %d of the class %s, object skipped at offset %d", version, cl->GetName(), Length() ); + --current; CheckByteCount(R__s, R__c, cl); return 0; } @@ -3963,6 +3999,9 @@ Int_t TBufferFile::ReadClassBuffer(const TClass *cl, void *pointer, const TClass ApplySequence(*(sinfo->GetReadObjectWiseActions()), (char*)pointer ); if (sinfo->TStreamerInfo::IsRecovered()) R__c=0; // 'TStreamerInfo::' avoids going via a virtual function. + if (current != fByteCountStack.size()) + Fatal("ReadClassBuffer", "We are out of sync at level %lld vs %zu for %s", current, fByteCountStack.size(), cl->GetName()); + --current; // Check that the buffer position corresponds to the byte count. CheckByteCount(R__s, R__c, cl); @@ -4031,8 +4070,11 @@ Int_t TBufferFile::ApplySequence(const TStreamerInfoActions::TActionSequence &se for(TStreamerInfoActions::ActionContainer_t::const_iterator iter = sequence.fActions.begin(); iter != end; ++iter) { + auto current = fByteCountStack.size(); (*iter).PrintDebug(*this,obj); (*iter)(*this,obj); + if (current != fByteCountStack.size()) + Fatal("ApplySequence", "We are out of sync starting at level %lld vs %zu", current, fByteCountStack.size()); } } else { @@ -4041,7 +4083,10 @@ Int_t TBufferFile::ApplySequence(const TStreamerInfoActions::TActionSequence &se for(TStreamerInfoActions::ActionContainer_t::const_iterator iter = sequence.fActions.begin(); iter != end; ++iter) { + auto current = fByteCountStack.size(); (*iter)(*this,obj); + if (current != fByteCountStack.size()) + Fatal("ApplySequence", "ByteCountStack is out of sync starting at level %zu vs %zu", current, fByteCountStack.size()); } } diff --git a/io/io/src/TContainerConverters.cxx b/io/io/src/TContainerConverters.cxx index 6958ad082e2fc..b74fe8b30f4b6 100644 --- a/io/io/src/TContainerConverters.cxx +++ b/io/io/src/TContainerConverters.cxx @@ -72,9 +72,8 @@ void TConvertClonesArrayToProxy::operator()(TBuffer &b, void *pmember, Int_t siz Int_t nobjects, dummy; char nch; TString s; - char classv[256]; - void *env; - UInt_t start, bytecount; + char classv[256]; + void *env; R__ASSERT(b.IsReading()); @@ -95,6 +94,9 @@ void TConvertClonesArrayToProxy::operator()(TBuffer &b, void *pmember, Int_t siz if (size==0) size=1; for(Int_t k=0; kCommit(env); b.CheckByteCount(start, bytecount,TClonesArray::Class()); + if (needAlloc) { + // Balance/check for the ReadClass byte count. + b.CheckByteCount(cntpos, tag, TClonesArray::Class()); + } } } diff --git a/net/net/src/TMessage.cxx b/net/net/src/TMessage.cxx index f36e8b296804c..4105aadcf9f41 100644 --- a/net/net/src/TMessage.cxx +++ b/net/net/src/TMessage.cxx @@ -392,10 +392,16 @@ Int_t TMessage::Compress() fCompPos = fBufCur; bufcur = fBufComp; + if (static_cast(CompLength()) > static_cast(std::numeric_limits::max())) { + Warning("Compress", "Compressed buffer length too large (%d)", CompLength()); + } tobuf(bufcur, (UInt_t)(CompLength() - sizeof(UInt_t))); Int_t what = fWhat | kMESS_ZIP; tobuf(bufcur, what); - tobuf(bufcur, Length()); // original uncompressed buffer length + if (static_cast(Length()) > static_cast(std::numeric_limits::max())) { + Warning("Compress", "Original buffer length too large (%zu)", Length()); + } + tobuf(bufcur, (UInt_t)Length()); // original uncompressed buffer length return 0; } diff --git a/tree/ntuple/test/CustomStruct.hxx b/tree/ntuple/test/CustomStruct.hxx index 0cde0ccb3e091..350305a28fc6c 100644 --- a/tree/ntuple/test/CustomStruct.hxx +++ b/tree/ntuple/test/CustomStruct.hxx @@ -497,8 +497,4 @@ struct AlignmentEnvelope { std::array fArr; }; -struct VersionedStreamerField { - ClassDefNV(VersionedStreamerField, 2); -}; - #endif diff --git a/tree/ntuple/test/CustomStructLinkDef.h b/tree/ntuple/test/CustomStructLinkDef.h index c8e6ec796cad9..4786ce21e2860 100644 --- a/tree/ntuple/test/CustomStructLinkDef.h +++ b/tree/ntuple/test/CustomStructLinkDef.h @@ -185,6 +185,5 @@ #pragma link C++ class AlignedAs+; #pragma link C++ class OverAligned+; #pragma link C++ class AlignmentEnvelope+; -#pragma link C++ class VersionedStreamerField+; #endif diff --git a/tree/ntuple/test/StreamerField.hxx b/tree/ntuple/test/StreamerField.hxx index cf5d29d4d26bc..dbf957d96cce4 100644 --- a/tree/ntuple/test/StreamerField.hxx +++ b/tree/ntuple/test/StreamerField.hxx @@ -101,4 +101,8 @@ struct TemperatureKelvin { float fValue; }; +struct VersionedStreamerField { + ClassDefNV(VersionedStreamerField, 2); +}; + #endif // ROOT_RNTuple_Test_StreamerField diff --git a/tree/ntuple/test/StreamerFieldLinkDef.h b/tree/ntuple/test/StreamerFieldLinkDef.h index 59010c53264e4..4d5b33de2e90e 100644 --- a/tree/ntuple/test/StreamerFieldLinkDef.h +++ b/tree/ntuple/test/StreamerFieldLinkDef.h @@ -26,5 +26,6 @@ #pragma link C++ options = rntupleStreamerMode(true) class TemperatureCelsius + ; #pragma link C++ options = rntupleStreamerMode(true) class TemperatureKelvin + ; +#pragma link C++ options = rntupleStreamerMode(true) class VersionedStreamerField + ; #endif diff --git a/tree/ntuple/test/rfield_streamer.cxx b/tree/ntuple/test/rfield_streamer.cxx index 1a331b5b195fb..41137cedad18e 100644 --- a/tree/ntuple/test/rfield_streamer.cxx +++ b/tree/ntuple/test/rfield_streamer.cxx @@ -412,10 +412,26 @@ namespace { /// Used to create on-disk streamer fields with different field versions class RVersionedStreamerField : public RFieldBase { +private: + class RVersionedStreamerFieldDeleter : public RDeleter { + private: + TClass *fClass = nullptr; + + public: + explicit RVersionedStreamerFieldDeleter(TClass *cl) : RDeleter(cl->GetClassAlignment()), fClass(cl) {} + void operator()(void *objPtr, bool dtorOnly) final + { + fClass->Destructor(objPtr, true /* dtorOnly */); + RDeleter::operator()(objPtr, dtorOnly); + } + }; + + TClass *fClass = nullptr; + protected: std::unique_ptr CloneImpl(std::string_view newName) const final { - return std::make_unique(newName, fCustomVersion); + return std::make_unique(newName, fClass->GetName(), fCustomVersion); } const RColumnRepresentations &GetColumnRepresentations() const final @@ -428,23 +444,27 @@ class RVersionedStreamerField : public RFieldBase { void GenerateColumns() final { GenerateColumnsImpl(); } void GenerateColumns(const ROOT::RNTupleDescriptor &) final {} - void ConstructValue(void *) const final {} + void ConstructValue(void *where) const final { fClass->New(where); } + std::unique_ptr GetDeleter() const final + { + return std::make_unique(fClass); + } std::size_t AppendImpl(const void *) final { return 0; } public: std::uint32_t fCustomVersion = 0; - RVersionedStreamerField(std::string_view name, std::uint32_t version) - : RFieldBase(name, "VersionedStreamerField", ROOT::ENTupleStructure::kStreamer, /*isSimple=*/false), - fCustomVersion(version) + RVersionedStreamerField(std::string_view name, std::string_view classname, std::uint32_t version) + : RFieldBase(name, classname, ROOT::ENTupleStructure::kStreamer, /*isSimple=*/false), + fClass(TClass::GetClass(std::string(classname).c_str())), fCustomVersion(version) { } std::uint32_t GetFieldVersion() const final { return fCustomVersion; } std::uint32_t GetTypeVersion() const final { return 137; } - std::size_t GetValueSize() const final { return 0; } - std::size_t GetAlignment() const final { return 0; } + std::size_t GetValueSize() const final { return fClass->GetClassSize(); } + std::size_t GetAlignment() const final { return fClass->GetClassAlignment(); } }; } // anonymous namespace @@ -455,7 +475,7 @@ TEST(RField, StreamerFieldVersion) FileRaii fileGuard("test_ntuple_rfield_streamer_version.root"); { auto model = RNTupleModel::Create(); - model->AddField(std::make_unique("f", version)); + model->AddField(std::make_unique("f", "VersionedStreamerField", version)); auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); } auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath());