TTree: Prevent access to already deleted scratch area #19938
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug in TTree where previously read data from a deleted scratch area could be accessed incorrectly for associative containers. The fix involves preventing duplicate reads on associative containers when the same entry is accessed multiple times, and ensures proper reset of read entry cursors for sub-branches.
Key changes:
- Added helper function to identify associative containers and prevent duplicate reads
- Modified entry reading logic to handle associative containers specially
- Added recursive reset functionality for sub-branch read cursors
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| void RecursiveResetReadEntry(TBranch *br) { | ||
| br->ResetReadEntry(); | ||
| for(auto sub : *br->GetListOfBranches()) | ||
| RecursiveResetReadEntry((TBranch*)sub); |
There was a problem hiding this comment.
The C-style cast to TBranch* should be replaced with a static_cast for better type safety and clarity.
| RecursiveResetReadEntry((TBranch*)sub); | |
| RecursiveResetReadEntry(static_cast<TBranch*>(sub)); |
Copilot uses AI. Check for mistakes.
tree/tree/src/TBranchElement.cxx
Outdated
| // this currently actually prevents the reading of the sub-branches because | ||
| // the branch count GetEntry actually relies on this code portion to do the | ||
| // reading. |
There was a problem hiding this comment.
This comment is unclear and contains grammatical issues. Consider revising to: 'This currently prevents reading of sub-branches because the branch count GetEntry relies on this code section to perform the reading.'
| // this currently actually prevents the reading of the sub-branches because | |
| // the branch count GetEntry actually relies on this code portion to do the | |
| // reading. | |
| // This currently prevents reading of sub-branches because the branch count | |
| // GetEntry relies on this code section to perform the reading. |
Copilot uses AI. Check for mistakes.
Test Results 21 files 21 suites 3d 15h 10m 36s ⏱️ For more details on these failures, see this check. Results for commit bfba580. ♻️ This comment has been updated with latest results. |
Fix one case that had not been updated to support multimap/multiset.
pcanal
force-pushed
the
tbranchelement-associative
branch
from
bfba580 to
70af8e1
Compare
The flow for reading an STL associative container is:
(a) Read branch count
(b) load sub-branch data into a staging/temporary area
(c) copy/insert that data into the associative container
Previously doing a direct read like:
tree->GetBranch("map.second")->GetEntry(e);
was leading to 'write' into deleted memory.
This allow to read individual sub-branches that were written as part of map but are being read into a non-associative container (for example via an emulated proxy which for a map deprecated to vector like behavior)
pcanal
force-pushed
the
tbranchelement-associative
branch
from
70af8e1 to
4a7fa82
Compare
No description provided.