TLeaf::ReadBasket invalid write in TMVA test

[hahnjo]

After #10005 is merged, the test TMVA-CrossValidation-Serialise will fail due to a stack-use-after-return:

352: WRITE of size 4 at 0x7f7ff4e1b880 thread T0
352:     #0 0x7f800120c3b9 in frombuf(char*&, unsigned int*) /home/jhahnfel/ROOT/src/core/base/inc/Bytes.h:310:7
352:     #1 0x7f800120c2bc in frombuf(char*&, int*) /home/jhahnfel/ROOT/src/core/base/inc/Bytes.h:442:48
352:     #2 0x7f7ffe26e9d0 in TBufferFile::ReadInt(int&) /home/jhahnfel/ROOT/src/io/io/inc/TBufferFile.h:401:4
352:     #3 0x7f80011965fc in TLeafI::ReadBasket(TBuffer&) /home/jhahnfel/ROOT/src/tree/tree/src/TLeafI.cxx:165:9
352:     #4 0x7f8001055b29 in TBranch::ReadLeavesImpl(TBuffer&) /home/jhahnfel/ROOT/src/tree/tree/src/TBranch.cxx:2382:13
352:     #5 0x7f800106b4a9 in TBranch::GetEntry(long long, int) /home/jhahnfel/ROOT/src/tree/tree/src/TBranch.cxx:1691:4
352:     #6 0x7f800125eea0 in TTree::GetEntry(long long, int)::$_3::operator()() const /home/jhahnfel/ROOT/src/tree/tree/src/TTree.cxx:5628:23
352:     #7 0x7f800125d3c8 in TTree::GetEntry(long long, int) /home/jhahnfel/ROOT/src/tree/tree/src/TTree.cxx:5703:7
352:     #8 0x50a984 in TestContex::runApplicationPhase(TString) /home/jhahnfel/ROOT/src/tmva/tmva/test/crossvalidation/TestCrossValidationSerialise.cxx:188:13
352:     #9 0x50b2a3 in TestCvSerialise(TMVA::Types::EMVA, TString, TString) /home/jhahnfel/ROOT/src/tmva/tmva/test/crossvalidation/TestCrossValidationSerialise.cxx:231:7
352:     #10 0x50b6e2 in TestCrossValidationSerialise() /home/jhahnfel/ROOT/src/tmva/tmva/test/crossvalidation/TestCrossValidationSerialise.cxx:240:7
352:     #11 0x50b8b3 in main /home/jhahnfel/ROOT/src/tmva/tmva/test/crossvalidation/TestCrossValidationSerialise.cxx:255:4
352:     #12 0x7f7ffad0aca2 in __libc_start_main (/lib64/libc.so.6+0x3aca2)
352:     #13 0x42361d in _start (/home/jhahnfel/ROOT/build-clang-asan-debug/tmva/tmva/test/crossvalidation/testCrossValidationSerialise+0x42361d)
352:
352: Address 0x7f7ff4e1b880 is located in stack of thread T0 at offset 128 in frame
352:     #0 0x7f7feed1f30f in cling::LookupHelper::findType(llvm::StringRef, cling::LookupHelper::DiagSetting) const (/home/jhahnfel/ROOT/build-clang-asan-debug/lib/libCling.so+0xdd430f)

Note: There may be other problems in the test after this issue is fixed. Please check locally with an instrumented build that the test passes afterwards!

[hahnjo]

Note that I'm completely unsure if this is really Cling's fault. If not, we have to re-assign accordingly.

[Axel-Naumann]

This isn't cling; it's TLeaf reading an int into the wrong location. I'll need to debug which int that is; I suspect classID/I

[hahnjo]

Ok, can you update the issue title to something appropriate?

[pcanal]

Most likely the TTree is still pointing to older (user data) addresses. (The 'failing' function set the address of only 4 out 7 branches. i.e. missing a tree->ResetBranchAddresses(); somewhere

[hahnjo]

Still there, would be good if somebody could have a look.

[hahnjo]

ping @Axel-Naumann @lmoneta @pcanal this is still there

[hahnjo]

ping @Axel-Naumann @lmoneta @pcanal Jenkins and me can still reproduce...

[ferdymercury]

@hahnjo
I think the issue is maybe that this test uses:

static UInt_t id = 0;
TTree *data = new TTree(name, name);
data->Branch("EventNumber", &id, "EventNumber/I");

but later:

Float_t fevNum;
fReader.AddSpectator("EventNumber", &fevNum);
tree->SetBranchAddress("EventNumber", &fevNum);

In contrast, TestCrossValidationIntVar seems to correctly use two variables to properly read the tree and spectate:

   Int_t uid;
   Float_t fid;
   reader.AddSpectator("EventNumber", &fid);
   tree->SetBranchAddress("EventNumber", &uid);

[pcanal]

Valgrind complains of:

==24718== Invalid write of size 4
==24718==    at 0x5778F9D: frombuf(char*&, unsigned int*) (Bytes.h:310)
==24718==    by 0x5779004: frombuf(char*&, int*) (Bytes.h:442)
==24718==    by 0x5EF5256: TBufferFile::ReadInt(int&) (TBufferFile.h:401)
==24718==    by 0x575640B: TLeafI::ReadBasket(TBuffer&) (TLeafI.cxx:165)
==24718==    by 0x56F5344: TBranch::ReadLeavesImpl(TBuffer&) (TBranch.cxx:2465)
==24718==    by 0x56F2C0A: TBranch::GetEntry(long long, int) (TBranch.cxx:1753)
==24718==    by 0x5791D30: TTree::GetEntry(long long, int)::{lambda()#1}::operator()() const (TTree.cxx:5661)
==24718==    by 0x5792522: TTree::GetEntry(long long, int) (TTree.cxx:5736)
==24718==    by 0x10D951: TestContex::runApplicationPhase(TString) (TestCrossValidationSerialise.cxx:188)
==24718==    by 0x10DEDE: TestCvSerialise(TMVA::Types::EMVA, TString, TString) (TestCrossValidationSerialise.cxx:231)
==24718==    by 0x10E0A1: TestCrossValidationSerialise() (TestCrossValidationSerialise.cxx:240)
==24718==    by 0x10E137: main (TestCrossValidationSerialise.cxx:255)
==24718==  Address 0x1ffeffd2b0 is on thread 1's stack
==24718==  5088 bytes below stack pointer

[pcanal]

TLDR: TTree* TMVA::DataSet::GetTree is missing a call to tree->ResetBranchAddresses() at the end.

The interesting bit is that TBufferFile::ReadInt is being used even-though all the variables in the example (and indeed all their corresponding branches) are floats.

So the issue is that the address of the Int branch:

*Br    0 :classID   : classID/I                                              *

is still set to an address that uses to be on the stack (according to valgrind).

How can it be possible, you may ask. Well ... looking at the result of fOutputFile->ls("") we notice:

....
   OBJ: TTree   TestTree        TestTree : 0 at: 0x5555555eae70

even before the call to fOutputFile.Get("dataset/TestTree");

This means that at point the TTree is already in memory (and already setup) and this prior loading is the likely place of the setting of the address to a local variable.

It turns out that the TTree is created in TTree* TMVA::DataSet::GetTree at DataSet.cxx:628 and later we have:

   UInt_t cls;
...
   tree->Branch( "classID", &cls, "classID/I" );
...
   return tree;
}

At which point the tree is returns pointing to a bunch of now invalid memory location.
Using:

  tree->ResetBranchAddresses();
  return tree;
}

solves the problem.