[io] Improve checks when reading arrays with TBufferFile by dpiparo · Pull Request #21814 · root-project/root

dpiparo · 2026-04-07T15:34:09Z

No description provided.

pcanal · 2026-04-07T17:18:55Z

io/io/inc/TBufferFile.h

@@ -268,6 +269,11 @@ class TBufferFile : public TBufferIO {

 //---------------------- TBufferFile inlines ---------------------------------------

+//______________________________________________________________________________
+inline bool TBufferFile::IsBadCollLen(Int_t len, Int_t n) const {
+   return  n <= 0 || len <=0 || (size_t)len > (size_t)(fBufMax - fBufCur);


Side note: the value zero is a totally valid (not bad) value that just happens to be treated the same way as the invalid values (i.e. read nothing for the array)

Do you have a better name in mind? Maybe ShouldReadColl?

pcanal · 2026-04-07T17:26:40Z

io/io/src/TBufferFile.cxx

@@ -818,7 +822,7 @@ Int_t TBufferFile::ReadArray(Int_t *&ii)
   *this >> n;
   Int_t l = sizeof(Int_t)*n;

-   if (l <= 0 || l > fBufSize) return 0;
+   if (IsBadCollLen(l, n)) return 0;


Suggested change

if (IsBadCollLen(l, n)) return 0;

if (!n || ExceedsBufferCapacity<Int_t>(n)) return 0;

and remove the variable l entirely.

If preferring not template:

Suggested change

if (IsBadCollLen(l, n)) return 0;

if (!n || ExceedsBufferCapacity(n, sizeof(Int_t)) return 0;

Are you suggesting to change the checks previously in place?
Suppose l is dropped. What would happen to the case where n is positive, but l, equal to n*sizeof(type) is negative?

github-actions · 2026-04-07T22:07:53Z

Test Results

22 files 22 suites 3d 5h 1m 13s ⏱️
3 831 tests 3 779 ✅ 1 💤 51 ❌
75 608 runs 75 522 ✅ 18 💤 68 ❌

For more details on these failures, see this check.

Results for commit bc06948.

silverweed · 2026-04-08T06:48:04Z

io/io/inc/TBufferFile.h

@@ -268,6 +269,11 @@ class TBufferFile : public TBufferIO {

 //---------------------- TBufferFile inlines ---------------------------------------

+//______________________________________________________________________________
+inline bool TBufferFile::IsBadCollLen(Int_t len, Int_t n) const {


Can we have either better named parameters or a doc comment for their meaning?

absolutely.

silverweed · 2026-04-08T06:51:53Z

io/io/src/TBufferFile.cxx

@@ -290,6 +290,10 @@ void TBufferFile::ReadCharStar(char* &s)
   Int_t nch;
   *this >> nch;
   if (nch > 0) {
+      if (nch == std::numeric_limits<Int_t>::max()) {
+         Error("ReadCharStar", "The number of characters nch is equal to max int!");


I would rephrase this as something like "Cannot allocate buffer: maximum capacity exceeded (%d bytes)"

fair enough.

dpiparo · 2026-04-08T09:19:16Z

The comments above are relevant, and easy to address. Thanks. I think that the tests failures are due to an issue of the file, which was never detected before. This simple program, that can be run as a macro too, reveals it:

#include "TFile.h"
#include "TTree.h"

/*
Invalid read happens at
- file "root://eospublic.cern.ch//eos/root-eos/testfiles/atlasFlushed.root"
- tree "CollectionTree"
- branch m_TrigMuonEFInfoContainers.vector<TPObjRef>
- basket 23

#0  0x00007ffff448d02c in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007ffff443fb86 in raise () from /lib64/libc.so.6
#2  0x00007ffff4429873 in abort () from /lib64/libc.so.6
#3  0x00007ffff48a1b21 in __gnu_cxx::__verbose_terminate_handler() [clone .cold] () from /lib64/libstdc++.so.6
#4  0x00007ffff48ad53c in __cxxabiv1::__terminate(void (*)()) () from /lib64/libstdc++.so.6
#5  0x00007ffff48ad5a7 in std::terminate() () from /lib64/libstdc++.so.6
#6  0x00007ffff48ad809 in __cxa_throw () from /lib64/libstdc++.so.6
#7  0x00007ffff72c639b in TBufferFile::IsBadCollLen (this=0x9112330, len=256, n=64) at /foo/root/io/io/src/TBufferFile.cxx:68

Here: len is 256, fBufMax 152116592, fBufCur 152116338, fBufMax-fBufCur = 254: two extra bytes

#8  0x00007ffff72bbcea in TBufferFile::ReadArray (this=0x9112330, ii=@0x90e3358: 0x0) at /foo/root/io/io/src/TBufferFile.cxx:839
#9  0x00007ffff634314d in TBasket::ReadBasketBuffers (this=0x90e3280, pos=298023188, len=3586, file=0x1723640) at /foo/root/tree/tree/src/TBasket.cxx:671
#10 0x00007ffff6352a21 in TBranch::GetBasketImpl (this=0x31aec30, basketnumber=23, user_buffer=0x0) at /foo/root/tree/tree/src/TBranch.cxx:1267
#11 0x00007ffff6353198 in TBranch::GetBasketAndFirst (this=0x31aec30, basket=@0x7fffffffcc80: 0x0, first=@0x7fffffffcc78: 1100, user_buffer=0x0)
    at /foo/root/tree/tree/src/TBranch.cxx:1389
#12 0x00007ffff6353fb0 in TBranch::GetEntry (this=0x31aec30, entry=1100, getall=0) at /foo/root/tree/tree/src/TBranch.cxx:1715
#13 0x00007ffff6366ec4 in TBranchElement::GetEntry (this=0x31aec30, entry=1100, getall=0) at /foo/root/tree/tree/src/TBranchElement.cxx:2826
#14 0x00007ffff6366a34 in TBranchElement::GetEntry (this=0x31ae610, entry=1100, getall=0) at /foo/root/tree/tree/src/TBranchElement.cxx:2761
#15 0x00007ffff6366a34 in TBranchElement::GetEntry (this=0x31adfd0, entry=1100, getall=0) at /foo/root/tree/tree/src/TBranchElement.cxx:2761
#16 0x00007ffff63e4347 in operator() (__closure=0x7fffffffd2b0) at /foo/root/tree/tree/src/TTree.cxx:5741
#17 0x00007ffff63e44d5 in TTree::GetEntry (this=0x13dd370, entry=1100, getall=0) at /foo/root/tree/tree/src/TTree.cxx:5819
#18 0x00000000004021e0 in main () at repro.cpp:9
(gdb) f 10
#10 0x00007ffff6352a21 in TBranch::GetBasketImpl (this=0x31aec30, basketnumber=23, user_buffer=0x0) at /foo/root/tree/tree/src/TBranch.cxx:1267

*/

int repro()
{
   auto f = std::unique_ptr<TFile>{TFile::Open("root://eospublic.cern.ch//eos/root-eos/testfiles/atlasFlushed.root")};
   auto t = f->Get<TTree>("CollectionTree");

   // not strictly necessary, but just to illustrate the matter
   t->SetBranchStatus("*",0);
   t->SetBranchStatus("m_TrigMuonEFInfoContainers.vector<TPObjRef>", 1);

   t->GetEntry(1100); // Invalid Read!

   return 0;
}

int main(){return repro();}

@Sebasteuo

thanks to @Sebasteuo

@Sebasteuo

i.e. in the TBufferFile::Read*Array* methods. Previously, the collection length was checked to be positive and that it was less than the total buffer size. Those checks have been improved with a battery of 3 checks: 1. The collection's number of elements is checked to be positive 2. The collection length is checked to be positive 3. The collection length is checked to be less than the portion of the buffer that remains to be read The test roottest-root-io-prefetching-run had to be modified as well because of what seems to be a corrupted input file. thanks also to @Sebasteuo

dpiparo requested review from hahnjo and silverweed April 7, 2026 15:34

dpiparo self-assigned this Apr 7, 2026

dpiparo requested a review from pcanal as a code owner April 7, 2026 15:34

dpiparo added the in:I/O label Apr 7, 2026

pcanal reviewed Apr 7, 2026

View reviewed changes

silverweed reviewed Apr 8, 2026

View reviewed changes

[io] Check the number of characters in ReadCharStar

1e8be47

thanks to @Sebasteuo

dpiparo force-pushed the fix_tbuffer branch 2 times, most recently from b224ca1 to 6cbd362 Compare April 8, 2026 14:04

dpiparo force-pushed the fix_tbuffer branch from 6cbd362 to e50cef7 Compare April 8, 2026 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[io] Improve checks when reading arrays with TBufferFile#21814

[io] Improve checks when reading arrays with TBufferFile#21814
dpiparo wants to merge 2 commits intoroot-project:masterfrom
dpiparo:fix_tbuffer

dpiparo commented Apr 7, 2026

Uh oh!

pcanal Apr 7, 2026 •

edited

Loading

Uh oh!

dpiparo Apr 8, 2026

Uh oh!

pcanal Apr 7, 2026

Uh oh!

dpiparo Apr 8, 2026

Uh oh!

github-actions bot commented Apr 7, 2026

Uh oh!

silverweed Apr 8, 2026

Uh oh!

dpiparo Apr 8, 2026

Uh oh!

silverweed Apr 8, 2026

Uh oh!

dpiparo Apr 8, 2026

Uh oh!

dpiparo commented Apr 8, 2026 •

edited by hahnjo

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (IsBadCollLen(l, n)) return 0;
	if (!n \|\| ExceedsBufferCapacity<Int_t>(n)) return 0;

	if (IsBadCollLen(l, n)) return 0;
	if (!n \|\| ExceedsBufferCapacity(n, sizeof(Int_t)) return 0;

Conversation

dpiparo commented Apr 7, 2026

Uh oh!

pcanal Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpiparo Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

pcanal Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

dpiparo Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 7, 2026

Test Results

Uh oh!

silverweed Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dpiparo Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

silverweed Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dpiparo Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dpiparo commented Apr 8, 2026 • edited by hahnjo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pcanal Apr 7, 2026 •

edited

Loading

dpiparo commented Apr 8, 2026 •

edited by hahnjo

Loading