[jmcarcell]

This Pull request:

Changes or fixes:

TBranchProxy owns the collection proxy it creates with Generate() in Setup(), but only deletes it in Reset(). It is leaked when the TBranchProxy is destroyed, and when Setup() runs again after a TChain switches files. This happens when reading split collections with TTreeReaderArray (and therefore RDataFrame), e.g.:

Direct leak of 5640 byte(s) in 15 object(s) allocated from:
    #0 operator new(unsigned long)
    #1 TEmulatedCollectionProxy::Generate() const
    #2 ROOT::Detail::TBranchProxy::Setup()

fCollection is now a std::unique_ptr. The deleter calls PopProxy() before deleting, because ~TEmulatedCollectionProxy() clears the collection that is still pushed into the proxy. After a TChain file switch, that collection belongs to a tree that has already been deleted. Deleting without popping hangs or crashes in TEmulatedCollectionProxy::Shrink() when reading a split collection of a class without a dictionary over a chain of several files. The member is marked as transient since I/O doesn't support unique_ptrs with custom deleters.

Checklist:

• tested changes locally
• updated the docs (if necessary)

I tested this with LeakSanitizer (no leaks from Generate() remain, and the values read are the same as before) and with the tree and dataframe tests.

I haven't added a test for the leak itself: the ASan CI runs with detect_leaks=0, so it would never fail. Instead, I could add a regression test for the reason the custom deleter is needed: a roottest pair in roottest/root/tree/reader, following CreateWarnNoDictFile.C/WarnNoDict.C. A macro compiled with ACLiC writes two files with a split std::vector of a struct. A second macro, run without that dictionary so that the collection proxy is emulated, reads one of its data members with TTreeReaderArray over a TChain of both files and checks the values. Without PopProxy() in the deleter, this test hangs or, in the ASan build, reports a heap-use-after-free.

[vepadulano]

Thanks a lot @jmcarcell ! In principle LGTM, but I think having the extra test case would be helpful anyway, could you upload that too?

[jmcarcell]

Test added, note that it is not for the leak, which is not tested for in CI. It hangs without PopProxy() in the custom deleter.

[vepadulano]

Thanks! I believe we should wait for @pcanal 's opinion first. Meanwhile I'll start the CI 👍

[vepadulano]

This is mostly a question for @pcanal , I wonder if with this syntax we run the risk of leaving artifacts in the test directory that the reading test might pick up and effectively still read a dictionary from? The ChainNoDict.C test already has a good check at lines 5-6, but what's your opinion?

[jmcarcell]

ACLiC doesn't generate a rootmap here, only CreateChainNoDictFiles_C.so, _C.d and _C_ACLiC_dict_rdict.pcm, so there is nothing that would trigger autoloading of the library when reading. I checked by running ChainNoDict() in the test directory with those files present: after reading both files ChainNoDictHit still has no dictionary and the library isn't loaded.

[github-actions]

Test Results

    24 files      24 suites   3d 23h 19m 48s ⏱️
 3 898 tests  3 897 ✅ 0 💤 1 ❌
83 462 runs  83 461 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit ca56ea2.

♻️ This comment has been updated with latest results.