Invalid Read in TTree::Fill after SetBranchAddress

[daritter]

Describe the bug

Then changing the Address of a Branch in TTree it seems to sometimes still use the old memory address after the call. As a minimal example here we create a TNamed, set it as branch but directly after delete it and set the branch address to nullptr before filling:

#include <TTree.h>
#include <TFile.h>

#include <iostream>

int main() {
    auto* tf = TFile::Open("test.root", "RECREATE");
    auto* t = new TTree("tree", "tree");

    TNamed *f = new TNamed("foo", "bar");
    auto* b = t->Branch("FileMetaData", &f);
   
    delete f;
    f = nullptr;
    b->SetAddress(nullptr);

    t->Fill();
    t->Write();
    tf->Close();
    return 0;
}

When running this with address sanitizer it gives the following error with 6.24:

g++ -g `root-config --cflags --libs` -fsanitize=address -o mwe mwe.C && ./mwe 
=================================================================
==934566==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000112a68 at pc 0x7fcd23e920f0 bp 0x7ffcc8579ab0 sp 0x7ffcc8579aa8
READ of size 4 at 0x606000112a68 thread T0
    #0 0x7fcd23e920ef in int TStreamerInfoActions::WriteBasicType<unsigned int>(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*) root/io/io/src/TStreamerInfoActions.cxx:252
    #1 0x7fcd239c8950 in TStreamerInfoActions::TConfiguredAction::operator()(TBuffer&, void*) const root/io/io/inc/TStreamerInfoActions.h:123
    #2 0x7fcd239c8950 in TBufferFile::ApplySequence(TStreamerInfoActions::TActionSequence const&, void*) root/io/io/src/TBufferFile.cxx:3572
    #3 0x7fcd21fdd5dc in TBranch::FillImpl(ROOT::Internal::TBranchIMTHelper*) root/tree/tree/src/TBranch.cxx:891
    #4 0x7fcd22016bfa in TBranchElement::FillImpl(ROOT::Internal::TBranchIMTHelper*) root/tree/tree/src/TBranchElement.cxx:1265
    #5 0x7fcd22016441 in TBranchElement::FillImpl(ROOT::Internal::TBranchIMTHelper*) root/tree/tree/src/TBranchElement.cxx:1290
    #6 0x7fcd22016441 in TBranchElement::FillImpl(ROOT::Internal::TBranchIMTHelper*) root/tree/tree/src/TBranchElement.cxx:1290
    #7 0x7fcd221d81a7 in TTree::Fill() root/tree/tree/src/TTree.cxx:4586
    #8 0x4025f7 in main mwe.C:17
    #9 0x7fcd1f9b3bf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
    #10 0x402239 in _start (mwe+0x402239)

0x606000112a68 is located 8 bytes inside of 64-byte region [0x606000112a60,0x606000112aa0)
freed by thread T0 here:
    #0 0x7fcd250835c7 in operator delete(void*) ([...]/lib/libasan.so.6+0xae5c7)
    #1 0x402501 in main mwe.C:13
    #2 0x7fcd1f9b3bf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)

previously allocated by thread T0 here:
    #0 0x7fcd25082bf7 in operator new(unsigned long) ([...]/lib/libasan.so.6+0xadbf7)
    #1 0x7fcd247f9b81 in TStorage::ObjectAlloc(unsigned long) root/core/base/src/TStorage.cxx:330
    #2 0x402948 in TObject::operator new(unsigned long) [...]/root/include/TObject.h:167
    #3 0x40241e in main mwe.C:10
    #4 0x7fcd1f9b3bf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)

SUMMARY: AddressSanitizer: heap-use-after-free root/io/io/src/TStreamerInfoActions.cxx:252 in int TStreamerInfoActions::WriteBasicType<unsigned int>(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*)
Shadow bytes around the buggy address:
  0x0c0c8001a4f0: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c8001a500: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0c8001a510: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
  0x0c0c8001a520: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fa
  0x0c0c8001a530: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
=>0x0c0c8001a540: fd fd fd fd fd fd fd fa fa fa fa fa fd[fd]fd fd
  0x0c0c8001a550: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c8001a560: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0c8001a570: fd fd fd fd fd fd fd fd fa fa fa fa 00 00 00 00
  0x0c0c8001a580: 00 00 00 00 fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c8001a590: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==934566==ABORTING

Expected behavior

Expected behavior is for ROOT to realize the branch address has been changed to nullptr and create an internal buffer, not read from the freed memory area.

Interestingly it doesn't seem to have a problem without calling SetAddress as it then realizes it points to nullptr and will create a new object and modify f

To Reproduce

Run the above with ROOT compiled with -Dasan=ON

Setup

• ROOT 6.24.0
• GCC 10.2
• Ubuntu 18.04

Additional context

We didn't see this with ROOT 6.20 but I also did not specifically test it with address sanitizer so it might have been there already and just not cause any problems.

[hahnjo]

Can still reproduce:

==340274==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000253a68 at pc 0x7f7939a95968 bp 0x7ffca068c8c0 sp 0x7ffca068c8b0
READ of size 4 at 0x606000253a68 thread T0
    #0 0x7f7939a95967 in int TStreamerInfoActions::WriteBasicType<unsigned int>(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*) /home/jhahnfel/ROOT/src/io/io/src/TStreamerInfoActions.cxx:253:14
    #1 0x7f7939703247 in TStreamerInfoActions::TConfiguredAction::operator()(TBuffer&, void*) const /home/jhahnfel/ROOT/src/io/io/inc/TStreamerInfoActions.h:123:17
    #2 0x7f7939703247 in TBufferFile::ApplySequence(TStreamerInfoActions::TActionSequence const&, void*) /home/jhahnfel/ROOT/src/io/io/src/TBufferFile.cxx:3579:10
    #3 0x7f79383569a1 in TBranch::FillImpl(ROOT::Internal::TBranchIMTHelper*) /home/jhahnfel/ROOT/src/tree/tree/src/TBranch.cxx:891:7
    #4 0x7f793837de4f in TBranchElement::FillImpl(ROOT::Internal::TBranchIMTHelper*) /home/jhahnfel/ROOT/src/tree/tree/src/TBranchElement.cxx:1265:28
    #5 0x7f793837df3e in TBranchElement::FillImpl(ROOT::Internal::TBranchIMTHelper*) /home/jhahnfel/ROOT/src/tree/tree/src/TBranchElement.cxx:1290:30
    #6 0x7f793837df3e in TBranchElement::FillImpl(ROOT::Internal::TBranchIMTHelper*) /home/jhahnfel/ROOT/src/tree/tree/src/TBranchElement.cxx:1290:30
    #7 0x7f793848fe58 in TTree::Fill() /home/jhahnfel/ROOT/src/tree/tree/src/TTree.cxx:4607:24
    #8 0x514e84 in main /home/jhahnfel/ROOT/build-asan-clang/mwe.cpp:17:8
    #9 0x7f7935f71d84 in __libc_start_main (/lib64/libc.so.6+0x3ad84) (BuildId: f65c85bfdb904b623d4fe2139b4d7c25cf8c0b58)
    #10 0x42025d in _start (/home/jhahnfel/ROOT/build-asan-clang/mwe+0x42025d) (BuildId: 67ff975bd256a4f2e9f1123cc71a34e2e618f3a1)

0x606000253a68 is located 8 bytes inside of 64-byte region [0x606000253a60,0x606000253aa0)
freed by thread T0 here:
    #0 0x5124c8 in operator delete(void*) (/home/jhahnfel/ROOT/build-asan-clang/mwe+0x5124c8) (BuildId: 67ff975bd256a4f2e9f1123cc71a34e2e618f3a1)
    #1 0x7f793a34af9f in TObject::operator delete(void*) /home/jhahnfel/ROOT/src/core/base/src/TObject.cxx:1078:7
    #2 0x514d92 in main /home/jhahnfel/ROOT/build-asan-clang/mwe.cpp:13:5
    #3 0x7f7935f71d84 in __libc_start_main (/lib64/libc.so.6+0x3ad84) (BuildId: f65c85bfdb904b623d4fe2139b4d7c25cf8c0b58)

previously allocated by thread T0 here:
    #0 0x5119a8 in operator new(unsigned long) (/home/jhahnfel/ROOT/build-asan-clang/mwe+0x5119a8) (BuildId: 67ff975bd256a4f2e9f1123cc71a34e2e618f3a1)
    #1 0x7f793a39a83e in TStorage::ObjectAlloc(unsigned long) /home/jhahnfel/ROOT/src/core/base/src/TStorage.cxx:330:19
    #2 0x5150d4 in TObject::operator new(unsigned long) /home/jhahnfel/ROOT/build-asan-clang/./include/TObject.h:181:46
    #3 0x514c6d in main /home/jhahnfel/ROOT/build-asan-clang/mwe.cpp:10:17
    #4 0x7f7935f71d84 in __libc_start_main (/lib64/libc.so.6+0x3ad84) (BuildId: f65c85bfdb904b623d4fe2139b4d7c25cf8c0b58)

SUMMARY: AddressSanitizer: heap-use-after-free /home/jhahnfel/ROOT/src/io/io/src/TStreamerInfoActions.cxx:253:14 in int TStreamerInfoActions::WriteBasicType<unsigned int>(TBuffer&, void*, TStreamerInfoActions::TConfiguration const*)

[ferdymercury]

To complement the info, here is the path followed when Branch() is called, which sets deeply inside the pointer to the TNamed object that is then later (corruptly) read:

1  TBranchElement::Unroll                                                            TBranchElement.cxx 6372 0x7fbbcd16e6d2 
2  TBranchElement::Init                                                              TBranchElement.cxx 456  0x7fbbcd133c49 
3  TBranchElement::TBranchElement                                                    TBranchElement.cxx 262  0x7fbbcd131ec9 
4  TBranchElement::Unroll                                                            TBranchElement.cxx 6170 0x7fbbcd16ce72 
5  TTree::BronchExec                                                                 TTree.cxx          2586 0x7fbbcd285935 
6  TTree::Bronch                                                                     TTree.cxx          2407 0x7fbbcd28470e 
7  TTree::Branch                                                                     TTree.cxx          2025 0x7fbbcd282660 
8  TTree::BranchImp                                                                  TTree.cxx          1629 0x7fbbcd27f4a9 
9  TTree::Branch<TNamed>                                                             TTree.h            372  0x7fbbeb663f00 
10 invRead                                                                           invRead.C          11   0x7fbbeb6638e9 
11 ??                                                                                                        0x7fbbefa8002d 
12 ??                                                                                                        0x7fbc199f5940 
13 ??                                                                                                        0x615000002d80 
14 ??                                                                                                        0x7ffc3fc9a130 
15 cling::IncrementalExecutor::executeWrapper(llvm::StringRef, cling::Value *) const                         0x7fbbf38f35c3 

I triead, and the heap-after-free-error can be solved by changing in TStreamerInfoActions:255

buf << *x;

with

buf << (x ? *x : T{});

[pcanal]

TStreamerInfoActions:255

This is the inner most part of the I/O and the most commonly uses code. Consequently this will (should) have a very severe impact on performance.

Furthermore, that (inner) function contract is that it is being passed a valid address.

The problem (to be solved) is that

    b->SetAddress(nullptr);

seems to be ignored. It should lead to the internal of TBranchElement to now point to a newly created (as a result of this code) object (of the right type) owned by the branch.

Similarly, we ought to detect (this time we can probably just error out) the case:

    f = nullptr;
    t->Fill();

early in the FIll stack, maybe as soon:

#6 0x7fcd22016441 in TBranchElement::FillImpl(ROOT::Internal::TBranchIMTHelper*) root/tree/tree/src/TBranchElement.cxx:1290