Skip to content

[tree] In TBasket/TBranch, error out if reading oob in the streamer #22165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
silverweed wants to merge 2 commits into
base: master
Choose a base branch
from
+47 −3
Open

[tree] In TBasket/TBranch, error out if reading oob in the streamer #22165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e33a86
[tree] In TBasket/TBranch, error out if reading oob in the streamer
silverweed May 6, 2026
c789f0f
Add integration test for TBasket
silverweed May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions roottest/root/tree/basket/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,11 @@ ROOTTEST_ADD_TEST(basket
MACRO rundropbasket.C
COPY_TO_BUILDDIR DataTest3_cel.root
OUTREF dropbasket.ref)

# corrupted.root contains a TTree "tree" with a corrupted TBasket that has fNevBufSize == 4 and fNevBuf == 4210752.
# Since fNevBuf is much larger than fNevBufSize this would cause oob reads unless TBasket properly validates this.
# Verify that it does.
ROOTTEST_ADD_TEST(corrupted_basket
COPY_TO_BUILDDIR corrupted.root
COMMAND ${ROOT_root_CMD} -q -l -e "(new TFile(\"corrupted.root\"))->template Get<TTree>(\"tree\")->GetEntry(0)"
PASSREGEX "Inconsistent length for the entry offset buffer \\(4210752 events for a buffer size of 4\\)")
Binary file added roottest/root/tree/basket/corrupted.root
View file
Open in desktop
Binary file not shown.
34 changes: 31 additions & 3 deletions tree/tree/src/TBasket.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,16 +1005,44 @@ void TBasket::Streamer(TBuffer &b)
flag -= 80;
}
if (!mustGenerateOffsets && flag && (flag % 10 != 2)) {
// NOTE: fNevBuf is the number of entries stored in the basket, while fNevBufSize is the capacity of the
// fEntryOffset and fDisplacement arrays.
if (fNevBuf > fNevBufSize) {
Comment thread
silverweed marked this conversation as resolved.
Error(
"Streamer",
"Inconsistent length for the entry offset buffer (%d events for a buffer size of %d). Refusing to deserialize.",
fNevBuf, fNevBufSize);
MakeZombie();
return;
}
ResetEntryOffset();
fEntryOffset = new Int_t[fNevBufSize];
if (fNevBuf) b.ReadArray(fEntryOffset);
if (fNevBuf) {
// Alas, ReadArray will read the number of elements to store into fEntryOffset from the file, but it
// has no way of knowing whether we're passing a large-enough array.
// Therefore we prevent the problem altogether by ignoring fNevBufSize and just having ReadArray allocate
// the buffer for us. This way we are sure that it will be of the correct size even if the file contains
// corrupted data.
Comment on lines +1023 to +1024
Copy link
Copy Markdown
Member

@pcanal pcanal May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that we can do better. With this change we swap the potential problem that the actual number of elements read is larger than the allocated memory (leading to out of bound writes) for the potential problem that the actual number of elements read is smaller than the expected number of elements (leading to out of bounds reads).

Maybe a better way is to explicitly read the size and to use ReadFastArray so that we can do the proper check.

Copy link
Copy Markdown
Contributor Author

@silverweed silverweed May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to use the returned (actual) nElemsRead and compare it with the purported number. I believe that should ensure we read exactly however many entries we expected.

fEntryOffset = nullptr;
auto nElemsRead = b.ReadArray(fEntryOffset);
if (nElemsRead != fNevBufSize) {
Error(
"Streamer",
"Inconsistent length for the entry offset buffer (expected %d elements, read %d). Refusing to deserialize.",
fNevBufSize, nElemsRead);
MakeZombie();
return;
}
} else {
fEntryOffset = new Int_t[fNevBufSize];
}
if (20<flag && flag<40) {
for(int i=0; i<fNevBuf; i++){
fEntryOffset[i] &= ~kDisplacementMask;
}
}
if (flag>40) {
fDisplacement = new Int_t[fNevBufSize];
// ReadArray will allocate this for us.
fDisplacement = nullptr;
b.ReadArray(fDisplacement);
}
} else if (mustGenerateOffsets) {
Expand Down
8 changes: 8 additions & 0 deletions tree/tree/src/TBranch.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3136,6 +3136,14 @@ void TBranch::Streamer(TBuffer& b)
}
fBasketEntry = new Long64_t[fMaxBaskets];
b >> n;
if (n > fMaxBaskets) {
Error("Streamer",
"Inconsistent number of baskets. This basket cannot be read. Read %d for the actual number of baskets "
"while we read %d as the value of fMaxBaskets.",
n, fMaxBaskets);
MakeZombie();
return;
}
for (i=0;i<n;i++) {b >> ijunk; fBasketEntry[i] = ijunk;}
fBasketBytes = new Int_t[fMaxBaskets];
if (v > 4) {
Expand Down
Loading