From a6eeb8273430e26832193a0ec4b1525d560dc86d Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Thu, 25 Oct 2018 15:05:55 -0500 Subject: [PATCH 1/9] Resolve memory over-write due to execution of a I/O rule on wrong object. This prevent the inappropriate execution on a rule intent for an inner object on the outer object('s memory space) In a case where the top level branch is: 1 edm::Wrapper > > which contains 2 16, obj, vector simple base pat::PATObject 3 360, overlapItems_, vector > simple base edm::PtrVectorBase 4 48, cachedItems_, atomic*> ***TRANSIENT-WITH-RULE** The TStreamerInfo Action Sequence for (4) was being executed the obj branch/level The bug was that GatherArtificialElements would drill through (3) eventhough it was not split and it did so because it did not recognize there was a branch for it because it added (errorneously) the name of the base class in the branch prefix. --- tree/tree/src/TBranchElement.cxx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 65bf00b35ce11..177a08fa6904c 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -2015,7 +2015,15 @@ static void GatherArtificialElements(const TObjArray &branches, TStreamerInfoAct ids.back().fNestedIDs->fOwnOnfileObject = kTRUE; } ids.back().fNestedIDs->fOnfileObject = onfileObject; - GatherArtificialElements(branches, ids.back().fNestedIDs->fIDs, ename + ".", nextinfo, offset + nextel->GetOffset()); + TString subprefix; + if (prefix.Length() && nextel->IsA() == TStreamerBase::Class()) { + // We skip the name of the base class if there is already a prefix. + // See TBranchElement::Unroll + subprefix = prefix; + } else { + subprefix = ename + "."; + } + GatherArtificialElements(branches, ids.back().fNestedIDs->fIDs, subprefix, nextinfo, offset + nextel->GetOffset()); if (ids.back().fNestedIDs->fIDs.empty()) ids.pop_back(); } From 025ef702aea5b55b7179da208127509a425b48fc Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 28 Aug 2018 14:58:59 +0200 Subject: [PATCH 2/9] Remove left over work byproduct. Lock was moved deeper --- tree/tree/src/TBranchElement.cxx | 1 - 1 file changed, 1 deletion(-) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 177a08fa6904c..51cd4401714be 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -5295,7 +5295,6 @@ void TBranchElement::SetAddress(void* addr) // We do this only once because it depends only on // the type of our object, not on its address. if (!fInitOffsets) { - // R__LOCKGUARD(gInterpreterMutex); InitializeOffsets(); } From 842d00d9466d974eb74c9dc76aba82bcf15aabf8 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Thu, 1 Nov 2018 10:07:10 -0500 Subject: [PATCH 3/9] Properly record kMissing in branch's action sequence. I.e. insure we do not add or substract from the special offset value kMissing (which result in 'valid' offset in the 10,000 range leading to memory over-write or out-of-bounds reads) --- io/io/inc/TStreamerInfoActions.h | 2 ++ io/io/src/TStreamerInfoActions.cxx | 36 ++++++++++++++++++++++++++++-- tree/tree/inc/TBranchElement.h | 1 + tree/tree/src/TBranchElement.cxx | 25 ++++++++++++++++++++- 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/io/io/inc/TStreamerInfoActions.h b/io/io/inc/TStreamerInfoActions.h index 589fa03963b63..bc3d8a5d5133b 100644 --- a/io/io/inc/TStreamerInfoActions.h +++ b/io/io/inc/TStreamerInfoActions.h @@ -42,6 +42,7 @@ namespace TStreamerInfoActions { virtual ~TConfiguration() {}; virtual void AddToOffset(Int_t delta); + virtual void SetMissing(); virtual TConfiguration *Copy() { return new TConfiguration(*this); } @@ -193,6 +194,7 @@ namespace TStreamerInfoActions { ActionContainer_t fActions; void AddToOffset(Int_t delta); + void SetMissing(); TActionSequence *CreateCopy(); static TActionSequence *CreateReadMemberWiseActions(TVirtualStreamerInfo *info, TVirtualCollectionProxy &proxy); diff --git a/io/io/src/TStreamerInfoActions.cxx b/io/io/src/TStreamerInfoActions.cxx index ed10e9eda7a2e..1d1176001d5ae 100644 --- a/io/io/src/TStreamerInfoActions.cxx +++ b/io/io/src/TStreamerInfoActions.cxx @@ -63,7 +63,16 @@ namespace TStreamerInfoActions // Add the (potentially negative) delta to all the configuration's offset. This is used by // TBranchElement in the case of split sub-object. - fOffset += delta; + if (fOffset != TVirtualStreamerInfo::kMissing) + fOffset += delta; + } + + void TConfiguration::SetMissing() + { + // Add the (potentially negative) delta to all the configuration's offset. This is used by + // TBranchElement in the case of split sub-object. + + fOffset = TVirtualStreamerInfo::kMissing; } void TConfiguredAction::PrintDebug(TBuffer &buf, void *addr) const @@ -153,7 +162,14 @@ namespace TStreamerInfoActions // Add the (potentially negative) delta to all the configuration's offset. This is used by // TBranchElement in the case of split sub-object. - fOffset += delta; + if (fOffset != TVirtualStreamerInfo::kMissing) + fOffset += delta; + fObjectOffset = 0; + } + + void SetMissing() + { + fOffset = TVirtualStreamerInfo::kMissing; fObjectOffset = 0; } @@ -3588,6 +3604,7 @@ TStreamerInfoActions::TActionSequence *TStreamerInfoActions::TActionSequence::Cr } return sequence; } + void TStreamerInfoActions::TActionSequence::AddToOffset(Int_t delta) { // Add the (potentially negative) delta to all the configuration's offset. This is used by @@ -3603,6 +3620,21 @@ void TStreamerInfoActions::TActionSequence::AddToOffset(Int_t delta) } } +void TStreamerInfoActions::TActionSequence::SetMissing() +{ + // Add the (potentially negative) delta to all the configuration's offset. This is used by + // TBranchElement in the case of split sub-object. + + TStreamerInfoActions::ActionContainer_t::iterator end = fActions.end(); + for(TStreamerInfoActions::ActionContainer_t::iterator iter = fActions.begin(); + iter != end; + ++iter) + { + if (!iter->fConfiguration->fInfo->GetElements()->At(iter->fConfiguration->fElemId)->TestBit(TStreamerElement::kCache)) + iter->fConfiguration->SetMissing(); + } +} + TStreamerInfoActions::TActionSequence *TStreamerInfoActions::TActionSequence::CreateCopy() { // Create a copy of this sequence. diff --git a/tree/tree/inc/TBranchElement.h b/tree/tree/inc/TBranchElement.h index c49a4cf369197..52662b51bb000 100644 --- a/tree/tree/inc/TBranchElement.h +++ b/tree/tree/inc/TBranchElement.h @@ -228,6 +228,7 @@ class TBranchElement : public TBranch { virtual void SetBranchFolder() { SetBit(kBranchFolder); } virtual void SetClassName(const char* name) { fClassName = name; } virtual void SetOffset(Int_t offset); + virtual void SetMissing(); inline void SetParentClass(TClass* clparent); virtual void SetParentName(const char* name) { fParentName = name; } virtual void SetTargetClass(const char *name); diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 51cd4401714be..b8ee3d9d5d949 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -3523,7 +3523,7 @@ void TBranchElement::InitializeOffsets() // // Compensate for the i/o routines adding our local offset later. if (subBranch->fObject == 0 && localOffset == TStreamerInfo::kMissing) { - subBranch->SetOffset(TStreamerInfo::kMissing); + subBranch->SetMissing(); // We stil need to set fBranchOffset in the case of a missing // element so that SetAddress is (as expected) not called // recursively in this case. @@ -5398,6 +5398,11 @@ void TBranchElement::SetOffset(Int_t offset) // We need to make sure that the Read and Write action's configuration // properly reflect this value. + if (offset == TVirtualStreamerInfo::kMissing) { + SetMissing(); + return; + } + if (fReadActionSequence) { fReadActionSequence->AddToOffset(offset - fOffset); } @@ -5407,6 +5412,24 @@ void TBranchElement::SetOffset(Int_t offset) fOffset = offset; } +//////////////////////////////////////////////////////////////////////////////// +/// Set offset of the object (to which the data member represented by this +/// branch belongs) inside its containing object (if any) to mark it as missing. + +void TBranchElement::SetMissing() +{ + // We need to make sure that the Read and Write action's configuration + // properly reflect this value. + + if (fReadActionSequence) { + fReadActionSequence->SetMissing(); + } + if (fFillActionSequence) { + fFillActionSequence->SetMissing(); + } + fOffset = TVirtualStreamerInfo::kMissing; +} + //////////////////////////////////////////////////////////////////////////////// /// Set the sequence of actions needed to read the data out of the buffer. From 76e4cd209416147fcc9d1470c73a7b096fb669d1 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Mon, 5 Nov 2018 17:09:08 -0600 Subject: [PATCH 4/9] In TBranchElement::Print use the correct StreamerInfo in case of schema evolution --- tree/tree/src/TBranchElement.cxx | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index b8ee3d9d5d949..2618ce5ba074d 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -3657,9 +3657,17 @@ static void PrintElements(const TStreamerInfo *info, const TStreamerInfoActions: { for(auto &cursor : ids) { auto id = cursor.fElemID; - if (id >= 0) - info->GetElement(id)->ls(); - else if (cursor.fNestedIDs) { + if (id >= 0) { + auto el = info->GetElement(id); + if (el) + el->ls(); + else { + Error("TBranchElement::Print", "Element for id #%d not found in StreamerInfo for %s", + id, info->GetName()); + info->ls(); + TClass::GetClass("PFTauWith")->GetStreamerInfos()->ls(); + } + } else if (cursor.fNestedIDs) { Printf(" Within subobject of type %s offset = %d", cursor.fNestedIDs->fInfo->GetName(), cursor.fNestedIDs->fOffset); PrintElements(cursor.fNestedIDs->fInfo, cursor.fNestedIDs->fIDs); } @@ -3715,7 +3723,8 @@ void TBranchElement::Print(Option_t* option) const } else if (!fNewIDs.empty() && GetInfoImp()) { TStreamerInfo *localInfo = GetInfoImp(); if (fType == 3 || fType == 4) { - localInfo = (TStreamerInfo *)fClonesClass->GetStreamerInfo(); + // Search for the correct version. + localInfo = FindOnfileInfo(fClonesClass, fBranches); } PrintElements(localInfo, fNewIDs); Printf(" with read actions:"); From be1399cc87900735ba48dfd3e9b62f0ba413184a Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 13 Nov 2018 13:52:02 -0600 Subject: [PATCH 5/9] In BuildOld, don't make all unique_ptr as having -> in comment. unique_ptr can be nullptr too sometimes *and* anyway TStreamerInfo::Build does not make the same restriction. The mismatch lead to baffling error message like: Warning in : Cannot convert A::h from type: B to type: B, skip element This solves one of the problems seen in https://sft.its.cern.ch/jira/browse/ROOT-9702. --- io/io/src/TStreamerInfo.cxx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/io/io/src/TStreamerInfo.cxx b/io/io/src/TStreamerInfo.cxx index 1020cb51219b0..ead253b947fe2 100644 --- a/io/io/src/TStreamerInfo.cxx +++ b/io/io/src/TStreamerInfo.cxx @@ -1976,7 +1976,6 @@ void TStreamerInfo::BuildOld() std::string typeNameBuf; const char* dmType = nullptr; Bool_t dmIsPtr = false; - Bool_t isUniquePtr = false; TDataType* dt(nullptr); Int_t ndim = 0 ; //dm->GetArrayDim(); std::array maxIndices; // 5 is the maximum supported in TStreamerElement::SetMaxIndex @@ -2012,7 +2011,7 @@ void TStreamerInfo::BuildOld() Bool_t nameChanged; typeNameBuf = TClassEdit::GetNameForIO(dmType, TClassEdit::EModType::kNone, &nameChanged); if (nameChanged) { - isUniquePtr = dmIsPtr = TClassEdit::IsUniquePtr(dmType); + dmIsPtr = TClassEdit::IsUniquePtr(dmType); dmType = typeNameBuf.c_str(); } if ((isStdArray = TClassEdit::IsStdArray(dmType))){ // We tackle the std array case @@ -2257,7 +2256,7 @@ void TStreamerInfo::BuildOld() if (element->GetNewType() != -2) { if (dm) { if (dmIsPtr) { - if (isUniquePtr || strncmp(dm->GetTitle(),"->",2)==0) { + if (strncmp(dm->GetTitle(),"->",2)==0) { // We are fine, nothing to do. if (newClass->IsTObject()) { newType = kObjectp; From c2844957dc053f35635b1a73bcb8fca3e2837271 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Mon, 5 Nov 2018 20:03:00 -0600 Subject: [PATCH 6/9] Fix offset calculation (error in db24ec75844). This addresses part of ROOT-9762 --- tree/tree/src/TBranchElement.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 2618ce5ba074d..f43731e38fe71 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -5465,7 +5465,7 @@ void TBranchElement::SetActionSequence(TClass *originalClass, TStreamerInfo *loc if (fInitOffsets) { auto index = parent->fBranches.IndexOf(this); if (index >= 0) { - fReadActionSequence->AddToOffset( - parent->fBranchOffset[index] ); + actionSequence->AddToOffset( - parent->fBranchOffset[index] ); } } // else it will be done by InitOffsets } From 7afbfa8c906cf9210d0a0dd338305f337d4960bb Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 6 Nov 2018 11:23:27 -0600 Subject: [PATCH 7/9] Fix ROOT-9762 (TBranchElement offset calculation) When a rule is associated to the top level node of split collection, we must not add the branch offset, since the rule is about the content of the collection --- tree/tree/src/TBranchElement.cxx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index f43731e38fe71..315aa798ba5e7 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -5458,9 +5458,13 @@ void TBranchElement::SetActionSequence(TClass *originalClass, TStreamerInfo *loc if (!isSplitNode) fNewIDs.erase(fNewIDs.begin()); - else { + else if (fType != 3 && fType != 4) { // fObject has the address of the sub-object but the streamer action have // offset relative to the parent. + + // Note: We skipped this for the top node of split collection because the + // sequence is about the content, we need to review what happens where an + // action related to the collection itself will land. TBranchElement *parent = dynamic_cast(GetMother()->GetSubBranch(this)); if (fInitOffsets) { auto index = parent->fBranches.IndexOf(this); From 7764eacddff91fe2c2bd512efa7b49ec5058d568 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 6 Nov 2018 11:25:21 -0600 Subject: [PATCH 8/9] Collapse if statements --- tree/tree/src/TBranchElement.cxx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 315aa798ba5e7..4a9cf7a436c7f 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -5458,7 +5458,8 @@ void TBranchElement::SetActionSequence(TClass *originalClass, TStreamerInfo *loc if (!isSplitNode) fNewIDs.erase(fNewIDs.begin()); - else if (fType != 3 && fType != 4) { + + else if (fInitOffsets && fType != 3 && fType != 4) { // fObject has the address of the sub-object but the streamer action have // offset relative to the parent. @@ -5466,13 +5467,12 @@ void TBranchElement::SetActionSequence(TClass *originalClass, TStreamerInfo *loc // sequence is about the content, we need to review what happens where an // action related to the collection itself will land. TBranchElement *parent = dynamic_cast(GetMother()->GetSubBranch(this)); - if (fInitOffsets) { - auto index = parent->fBranches.IndexOf(this); - if (index >= 0) { - actionSequence->AddToOffset( - parent->fBranchOffset[index] ); - } - } // else it will be done by InitOffsets - } + + auto index = parent->fBranches.IndexOf(this); + if (index >= 0) { + actionSequence->AddToOffset( - parent->fBranchOffset[index] ); + } + } // else it will be done by InitOffsets } //////////////////////////////////////////////////////////////////////////////// From a47358036b848f775625fc3be724ac89b8ed96db Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 23 Oct 2018 16:05:04 -0500 Subject: [PATCH 9/9] Avoid setting fCollProxy multiple times. GetCollectionProxy during the setting of fCollProxy calls TBranchElement::GetInfoImp that in some cases sets fCollProxy and ends up recording it (sometimes) in the action sequence. When GetCollectionProxy sets it too (i.e. change it) there is now a disconnect between the branch and the action sequences that lead to the action sequence to used an unset collection proxy: Fatal in : Size> Logic error - no proxy object set. aborting --- tree/tree/src/TBranchElement.cxx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tree/tree/src/TBranchElement.cxx b/tree/tree/src/TBranchElement.cxx index 4a9cf7a436c7f..8b438acbd45a8 100644 --- a/tree/tree/src/TBranchElement.cxx +++ b/tree/tree/src/TBranchElement.cxx @@ -2403,6 +2403,14 @@ TVirtualCollectionProxy* TBranchElement::GetCollectionProxy() } else { // We are not a top-level branch. TVirtualStreamerInfo* si = thiscast->GetInfoImp(); + if (fCollProxy) { + // The GetInfo set fProxy for us, let's not + // redo it; the value of fCollProxy is possibly + // used/recorded is the actions sequences, so + // if we change it here, we would need to propagate + // the change. + return fCollProxy; + } TStreamerElement* se = si->GetElement(fID); cl = se->GetClassPointer(); }