TTree GetEntry fails when branch addresses set in function

Hi,

I’ve run into a problem where ROOT will segfault when GetEntry() is called if the two lines of code that initialize a branch in a TTree being read are moved to a different function. This seems to happen with root 6.10/08 and 6.12/06. Newer version of root (checked with 6.18/00) don’t crash, but the data never seems to fill when the test script is run, it looks like everything is simply initialized to 0.

The problem happens when I go from

Test *t = new Test();
tr->SetBranchAddress("testClass", &t);

to

Test *t = CreateTest(tr);
...
Test *CreateTest(TTree *tr)
{
   Test *module = new Test();
   tr->SetBranchAddress("testClass", &module);
   return module;
}

I attached a script that writes and reads a ttree using both methods. Any help figuring out why setting the branch address this way doesn’t work is appreciated.

ttreeTest.c (1.1 KB)

Cheers,
Adam

Welcome to the ROOT Forum! And you’re right, this is weird, maybe @Axel can take a look?
BTW, can you try with:

Test *CreateTest(TTree *tr)
{
   static Test *module = new Test();
   tr->SetBranchAddress("testClass", &module);
   return module;
}

(it works for me)

Hi,
the module variable the address of which you pass to SetBranchAddress is destroyed at the end of CreateTest (and its value is copied in to a different variable, also of type Test *, which is returned by the function). So the tree is left with an invalid pointer to pointer.

In short: that code has undefined behavior in it (a “use after delete” bug) so depending on the compiler or the compiler options etc. you might or might not get crashes (or simply unexpected outputs, etc).

Cheers,
Enrico

P.S.: Bertrand’s addition of static of course increases the lifetime of module so that the issue goes away. Running your program under valgrind --suppressions=$ROOTSYS/etc/valgrind-root.supp should show point out the bug.

1 Like

Hi,

Thanks for the help! By dynamically allocating the pointer to pointer like below I was able to keep it valid (I think).

Test *CreateTest(TTree *tr)
{
   Test **ptrToPtr = new Test*;
   *ptrToPtr = new Test();
   tr->SetBranchAddress("testClass", ptrToPtr);
   return *ptrToPtr;
}

Now you have a potential memory leak, but that works :smiley:

No wait, but you have to return the pointer to pointer, not the pointer, otherwise the thing you pass to SetBranchAddress still goes out of scope at the end of the function! Again valgrind --suppressions=... should give you an error.

So ptrToPtr will go out of scope at the end, but it I think it shouldn’t be deleted because it’s dynamically allocated so the memory should still be sitting happily on the heap. The returned value is then a copy of the pointer. At least I think so. So there’s definitely a memory leak of the ptrToPtr because I’ve lost all ability to access that bit of memory but that should be fine given the number of times my actual code will use this method :smiley:

Interestingly, (but probably due to my incompetence with valgrind) on 16.18/00 valgrind has no error when running the obviously broken code. The “fixed” code does throw two errors Syscall param write(buf) points to uninitialized byte(s) and Address 0xfdb3c40 is 0 bytes inside a block of size 8 alloc'd. That address is the address of the pointer to pointer (what you get if you cout << ptrToPtr).

For completeness, I ran valgrind with the command valgrind --suppressions=$ROOTSYS/etc/valgrind-root.supp root.exe -l -b -q ttreeTest.C where I added a function ttreeTest() { readTreeFails()} with the “fixed” code above to get the errors.

Test*& CreateTest(TTree *tr) {
  static Test *module = 0; // always the user is responsible for deleting it
  tr->SetBranchAddress("testClass", &module);
  return module;
}
  // ...
  Test*& t = CreateTest(tr); // always access the original pointer variable
  // t = 0; // see what happens if you uncomment this line
  // ...

ah yeah that’s correct, brain tired, sorry

1 Like

This will work, but only in the simple case that this function is called for a single branch. So if you were to extend the function to a tree with multiple branches of type Test, and wanted to pass the branch name at run-time (the actual use case I’m interested in but simplified out for the post) then the returned pointer will always point to the branch last set using the function because the memory is only initialized once for the static variable.

So any returned variable of this function will always point to whatever branch was last passed.

Test*& CreateTest(TString branchName, TTree *tr)
{
   static Test *module = 0;
   tr->SetBranchAddress(branchName, &module);
   return module;
}

The extended “fixed” code won’t suffer from this problem because new memory is allocated each call.

Test *CreateTest(TString branchName, TTree *tr)
{
   Test **ptrToPtr = new Test*;
   *ptrToPtr = new Test();
   tr->SetBranchAddress(branchName, ptrToPtr);
   return *ptrToPtr;
}

You MUST NOT reuse the pointer given to “SetBranchAddress”, unless you first set the currently using it branch to another pointer or you execute “ResetBranchAddress”.
Make sure that you ALWAYS reference the original pointer (its value may be changed at any moment by ROOT), not just its value returned by your function “at startup”.

So I think a more proper solution would look something like:

Test*& CreateTest(TString branchName, TTree *tr)
{
   Test **ptrToPtr = new Test*;
   *ptrToPtr = new Test();
   tr->SetBranchAddress(branchName, ptrToPtr);
   return *ptrToPtr;
}

That way what is returned is the reference to the original pointer (not a copy), in case ROOT changes it, and creating a static variable is avoided so at each function call a new pointer is allocated.

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.