Skip to content

Commit

Permalink
[treeplayer] Fix memory management of C-style array branches in test
Browse files Browse the repository at this point in the history 
The test was dynamically allocating the array data members of the `Data` struct, but never deallocating them. This commit polishes the `Data` struct definition and ensures proper management of the data members.

The previous way of writing data to the TTree was leading to a bad memory access in the ReadBasicPointer inlined function in TStreamerInfoReadBuffer.cxx while reading the `double*` array. In particular, the issue arises when accessing and then deallocating the array at the current index provided by the `TCompInfo` object.

```
Target 0: (repro.out) stopped.
(lldb)
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
    frame #0: 0x00000001044cf140 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(this=<unavailable>, b=<unavailable>, arr=<unavailable>, compinfo=<unavailable>, first=<unavailable>, last=<unavailable>, narr=<unavailable>, eoffset=<unavailable>, arrayMode=0) at TStreamerInfoReadBuffer.cxx:923:65 [opt]
   920 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong:   ReadBasicPointer(Long_t);  continue;
   921 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong64: ReadBasicPointer(Long64_t);  continue;
   922 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kFloat:  ReadBasicPointer(Float_t);  continue;
-> 923 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kDouble: ReadBasicPointer(Double_t);  continue;
   924 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUChar:  ReadBasicPointer(UChar_t);  continue;
   925 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUShort: ReadBasicPointer(UShort_t);  continue;
   926 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUInt:   ReadBasicPointer(UInt_t);  continue;
Target 0: (repro.out) stopped.
(lldb)
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
    frame #0: 0x00000001044cf184 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(TBuffer&, char** const&, TStreamerInfo::TCompInfo* const*, int, int, int, int, int) [inlined] TBuffer::BufferSize(this=0x000060e00010ef00) const at TBuffer.h:98:41 [opt]
   95  	   TObject *GetParent()  const;
   96  	   char    *Buffer()     const { return fBuffer; }
   97  	   char    *GetCurrent() const { return fBufCur; }
-> 98  	   Int_t    BufferSize() const { return fBufSize; }
   99  	   void     DetachBuffer() { fBuffer = nullptr; }
   100 	   Int_t    Length()     const { return (Int_t)(fBufCur - fBuffer); }
   101 	   void     Expand(Int_t newsize, Bool_t copy = kTRUE);  // expand buffer to newsize
Target 0: (repro.out) stopped.
(lldb) p fBufSize
(Int_t) 32008
(lldb) s
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
    frame #0: 0x00000001044cf194 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(this=<unavailable>, b=<unavailable>, arr=<unavailable>, compinfo=<unavailable>, first=<unavailable>, last=<unavailable>, narr=<unavailable>, eoffset=<unavailable>, arrayMode=0) at TStreamerInfoReadBuffer.cxx:923:65 [opt]
   920 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong:   ReadBasicPointer(Long_t);  continue;
   921 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kLong64: ReadBasicPointer(Long64_t);  continue;
   922 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kFloat:  ReadBasicPointer(Float_t);  continue;
-> 923 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kDouble: ReadBasicPointer(Double_t);  continue;
   924 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUChar:  ReadBasicPointer(UChar_t);  continue;
   925 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUShort: ReadBasicPointer(UShort_t);  continue;
   926 	         case TStreamerInfo::kOffsetP + TStreamerInfo::kUInt:   ReadBasicPointer(UInt_t);  continue;
Target 0: (repro.out) stopped.
(lldb) s
Process 13498 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbebebebebebebeae)
    frame #0: 0x0000000107bac674 libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) + 76
libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate:
->  0x107bac674 <+76>: casalb w8, w9, [x22]
    0x107bac678 <+80>: cmp    w8, #0x2
    0x107bac67c <+84>: b.ne   0x107bac6f4    ; <+204>
    0x107bac680 <+88>: mov    x8, #-0x100000000 ; =-4294967296
Target 0: (repro.out) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbebebebebebebeae)
  * frame #0: 0x0000000107bac674 libclang_rt.asan_osx_dynamic.dylib`__asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) + 76
    frame #1: 0x0000000107c0c444 libclang_rt.asan_osx_dynamic.dylib`wrap__ZdaPv + 232
    frame #2: 0x00000001044d4a60 libRIO.so`int TStreamerInfo::ReadBuffer<char**>(this=<unavailable>, b=<unavailable>, arr=<unavailable>, compinfo=<unavailable>, first=<unavailable>, last=<unavailable>, narr=<unavailable>, eoffset=<unavailable>, arrayMode=0) at TStreamerInfoReadBuffer.cxx:923:65 [opt]
    frame #3: 0x0000000103ffc888 libRIO.so`TStreamerInfoActions::GenericReadAction(buf=0x000060e00010ef00, addr=0x0000602000056bd0, config=0x0000604000149910) at TStreamerInfoActions.cxx:195:45
    frame root-project#4: 0x0000000103caa5ec libRIO.so`TStreamerInfoActions::TConfiguredAction::operator()(this=0x00006030001693f0, buffer=0x000060e00010ef00, object=0x0000602000056bd0) const at TStreamerInfoActions.h:123:17
    frame root-project#5: 0x0000000103ca9ef8 libRIO.so`TBufferFile::ApplySequence(this=0x000060e00010ef00, sequence=0x000060600011ac20, obj=0x0000602000056bd0) at TBufferFile.cxx:3702:10
    frame root-project#6: 0x00000001064bc570 libTree.so`TBranchElement::ReadLeavesMemberBranchCount(this=0x0000619000566380, b=0x000060e00010ef00) at TBranchElement.cxx:4603:6
    frame root-project#7: 0x0000000106455ce4 libTree.so`TBranch::GetEntry(this=0x0000619000566380, entry=0, getall=0) at TBranch.cxx:1753:4
    frame root-project#8: 0x00000001064a1764 libTree.so`TBranchElement::GetEntry(this=0x0000619000566380, entry=0, getall=0) at TBranchElement.cxx:2783:27
    frame root-project#9: 0x000000010739915c libTreePlayer.so`ROOT::Detail::TBranchProxy::Read(this=0x00006110000c9580) at TBranchProxy.h:163:42
    frame root-project#10: 0x0000000107649ba8 libTreePlayer.so`(anonymous namespace)::TObjectArrayReader::At(this=0x0000603000169900, proxy=0x00006110000c9580, idx=1) at TTreeReaderArray.cxx:176:22
    frame root-project#11: 0x000000010000c2e4 repro.out`ROOT::Internal::TTreeReaderArrayBase::UntypedAt(this=0x000000016fdfe740, idx=1) const at TTreeReaderArray.h:41:62
    frame root-project#12: 0x000000010000c200 repro.out`TTreeReaderArray<double>::At(this=0x000000016fdfe740, idx=1) at TTreeReaderArray.h:205:54
    frame root-project#13: 0x00000001000065e0 repro.out`TTreeReaderArray<double>::operator[](this=0x000000016fdfe740, idx=1) at TTreeReaderArray.h:207:44
    frame root-project#14: 0x0000000100007b48 repro.out`simpleTest() at repro.cpp:123:26
    frame root-project#15: 0x0000000100007e10 repro.out`main at repro.cpp:128:5
    frame root-project#16: 0x000000018c718274 dyld`start + 2840
```
  • Loading branch information
@vepadulano
vepadulano committed Sep 23, 2024
1 parent 962009b commit 93e27a8
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 23 deletions.
35 changes: 27 additions & 8 deletions tree/treeplayer/test/data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,33 @@
#define TTREEREADER_LEAF_TEST_DATA

#include <vector>
struct Data {
unsigned int fUSize;
int fSize;
double* fArray; //[fSize]
float* fUArray; //[fUSize]
std::vector<double> fVec;
Double32_t fDouble32;
Float16_t fFloat16;
class Data {
[[maybe_unused]] unsigned int fUSize{2};
[[maybe_unused]] int fSize{4};
double *fArray{nullptr}; //[fSize]
float *fUArray{nullptr}; //[fUSize]
std::vector<double> fVec{17., 18., 19., 20., 21., 22.};
[[maybe_unused]] Double32_t fDouble32{17.};
[[maybe_unused]] Float16_t fFloat16{44.};

public:
explicit Data(bool /*justToHaveAUserDefinedConstructor*/)
: fArray(new double[4]{12., 13., 14., 15.}), fUArray(new float[2]{42., 43.})
{
}
Data() = default;
Data(const Data &) = delete;
Data &operator=(const Data &) = delete;
Data(Data &&) = delete;
Data &operator=(Data &&) = delete;
~Data()
{
delete[] fArray;
fArray = nullptr;
delete[] fUArray;
fUArray = nullptr;
}
std::vector<double> &GetVecMember() { return fVec; }
};

struct V {
Expand Down
24 changes: 9 additions & 15 deletions tree/treeplayer/test/leafs.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,32 +84,26 @@ TEST(TTreeReaderLeafs, LeafListCaseA) {
EXPECT_EQ(Bool, *trBool);
}



std::unique_ptr<TTree> CreateTree() {
std::unique_ptr<TTree> CreateTree()
{
TInterpreter::EErrorCode error = TInterpreter::kNoError;
gInterpreter->ProcessLine("#include \"data.h\"", &error);
if (error != TInterpreter::kNoError)
return {};

Data data;
// explicit user-defined constructor to distinguish from I/O constructor
Data data{true};
auto &vecDataMember = data.GetVecMember();
auto tree = std::make_unique<TTree>("T", "test tree");
tree->Branch("Data", &data);
data.fArray = new double[4]{12., 13., 14., 15.};
data.fSize = 4;
data.fUArray = new float[2]{42., 43.};
data.fUSize = 2;
data.fVec = { 17., 18., 19., 20., 21., 22.};
data.fDouble32 = 17.;
data.fFloat16 = 44.;
tree->Fill();

data.fVec.clear();
data.fVec.resize(3210, 1001.f); // ROOT-8747
vecDataMember.clear();
vecDataMember.resize(3210, 1001.f); // ROOT-8747
tree->Fill();

data.fVec.clear();
data.fVec.resize(2, 42.f); // ROOT-8747
vecDataMember.clear();
vecDataMember.resize(2, 42.f); // ROOT-8747
tree->Fill();

tree->ResetBranchAddresses();
Expand Down

0 comments on commit 93e27a8

Please sign in to comment.