From 7e33a861f04d6caaea6c69f1a5133c1015a34c06 Mon Sep 17 00:00:00 2001 From: silverweed Date: Wed, 6 May 2026 17:40:08 +0200 Subject: [PATCH 1/2] [tree] In TBasket/TBranch, error out if reading oob in the streamer --- tree/tree/src/TBasket.cxx | 34 +++++++++++++++++++++++++++++++--- tree/tree/src/TBranch.cxx | 8 ++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/tree/tree/src/TBasket.cxx b/tree/tree/src/TBasket.cxx index a77855a498986..cd257ce9da8ba 100644 --- a/tree/tree/src/TBasket.cxx +++ b/tree/tree/src/TBasket.cxx @@ -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) { + 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. + 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 (2040) { - fDisplacement = new Int_t[fNevBufSize]; + // ReadArray will allocate this for us. + fDisplacement = nullptr; b.ReadArray(fDisplacement); } } else if (mustGenerateOffsets) { diff --git a/tree/tree/src/TBranch.cxx b/tree/tree/src/TBranch.cxx index a44cf713d1916..6e1edd13dc3b4 100644 --- a/tree/tree/src/TBranch.cxx +++ b/tree/tree/src/TBranch.cxx @@ -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> ijunk; fBasketEntry[i] = ijunk;} fBasketBytes = new Int_t[fMaxBaskets]; if (v > 4) { From c789f0f0a8deca15f216bdb39895c7e5c12f2d1d Mon Sep 17 00:00:00 2001 From: silverweed Date: Thu, 7 May 2026 13:39:21 +0200 Subject: [PATCH 2/2] Add integration test for TBasket --- roottest/root/tree/basket/CMakeLists.txt | 8 ++++++++ roottest/root/tree/basket/corrupted.root | Bin 0 -> 5800 bytes 2 files changed, 8 insertions(+) create mode 100644 roottest/root/tree/basket/corrupted.root diff --git a/roottest/root/tree/basket/CMakeLists.txt b/roottest/root/tree/basket/CMakeLists.txt index e4fdc3ac7555e..961c7ad24ffc0 100644 --- a/roottest/root/tree/basket/CMakeLists.txt +++ b/roottest/root/tree/basket/CMakeLists.txt @@ -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(\"tree\")->GetEntry(0)" + PASSREGEX "Inconsistent length for the entry offset buffer \\(4210752 events for a buffer size of 4\\)") diff --git a/roottest/root/tree/basket/corrupted.root b/roottest/root/tree/basket/corrupted.root new file mode 100644 index 0000000000000000000000000000000000000000..c5cc790571f868914b9a179bc3dd7fcdb803b01f GIT binary patch literal 5800 zcmeHLWl)^KmYuM#0f>SG z2C700VmJT*Bv=5TYjRUtB%|DVsh`yA@eR8tsL_9`_yGPmQVmppAO_(3>lw8g0D!6c z9O7g{;0E?}as^xSdHf0HU%vpDf99a@3S}OFf&>bG=K=ueoBwM-y6F2Tdt%gl9e@I= z_a9~PKU-^TT{$q+(Z&-9!SM93u>qh10D*tTFaR<#GN@MfPYxje3ZT)Sy`W?K^~v>T zFTlqr3u{bWU6hNze*RWQFL2m_01R}14=_%IIzjv9w#aio|`b~wX2P5h(-Y% z)D4wOAT3jQ@D1;Z9ACOCsQkfgaKVE%hLd4yJCI#;M$>}t&GP=i0vKIjRS>Z47En7r zuw7L-8KR26yz*8x06=8TohMA0NW)|CUg>eE&BT*Sm6z?^Ua7pyO}9SW7)G!BIxi2G z~P(<4aMk2*d}`IC>k|sD^?Yn9Urlm4j`7J z7_#F7aGT*n$i~E~4zF0F1&6eQ%jchkF;@3Sv-*d~=zhpm$I;XT8f#=f*Pe;=yvNJ7 zdfzPH+@wm<)(F%F?o4_gZ|d|tdz#)C*q&}NpFRA-A@v~Ae0YFH7qdazgVUQyWATRY zg6KEeN|ZN2LMhH6rYBT9TRA0^rRN%r=(a7VH_UoeOYk_rv0hBq-tK9 zB_EfjqKzs~sSlJ@dFHf7hYprf{l4fgNjA;TzEwhFpM6UTX79mevB^bKe4q`aJVKB~ zblk3Dw_Rb9Npm3Js#=Oj`u2eB#f7JJcJ%3Q-^kDWI^XlkIm(6}Hf+1s$-G<944jmo z_VN#LdAHpA%0$^}OHWV2_>8$@Y4qF6kR3w78UEF}GK1KZy!ulj{xwqXym7139|61T z*0lbPxAPp%gF2T~I-MWP2DLt(l5~6PLug>dlQAoW>D5v8{wI*!B3ceU&bm!j+4XC! zeLt7DqeiXWJg&k+QFF=7FS68#J0A$Ozst>y^!1$PoQPi`8>OvVexb~!NYrxoiEqeD zGMtMAVyx&S$TP~xNxt9iErM}j+LC&>Qspi;e)mHqflu={$)fm#1V2~4ye@h9tUUhP z2?N2=CjCqpi;K+RU7Psvwe2P!wFfKX#Y4ur6*iHsN8)SUA9S0I>0M-;@0Hqa%V@N$ z=32)1W^zqTj49U47_dRv1mN zB|7mOil|bNzALu)Hf2Is8SYo#jOkqS*l$HbQ3tr0KA(&o6M0TdMc@r|X=c$aM!vW+ zteEOLE@BA2UM`hT_;XXs>vZHfE7DW%v1C^7xl=h$8r=~=(stqMH+30Rf&w)(72JZH zN=DEP{&F+|2i|0?eewg6e2lK2HpRw>1_bV?UaFMO4Q<`o%iBGrr?ys!>A=$&}hBE@Eee zEBMl07VHVJ4t`Oo&ze+2DANzM={~M~nFP3pZgF|TTto}y&6fq?W|MXc!9a~ZWQuUH zw6hM~Ddm9KflqG7fI-Zhd&~6X%@$R34pxm~1TwA&ditKRjbl?1e8ui`diT3%EBXT> z@Ot&SqR~XZ<}}^FTNuv~s#U#++3)%=u?)Yz45xqFInn>tYT)7rn9Drp;!fc+6yfDr zOHlwE0*+8lu-3|0CQp0fi~M9zvz0}#=c;kNR(rjo?=EtBSu1-1lD!a;-JaX=y4KW` z?`OPBGIQ1%uC}&xX?m;{gVt578!U>Jr!1qGmc%vCJJ}UB@hGF~WeQRd>z8xy^4pHZ zFB5q%HjIr23_9*M!-<9LF^ zoq@oNFtq&KVH+xUw|s7bGHQYujLw))@O0_|Uys|fsEI~OK8ze4A{!3vk5(j$mS$9O z?)*htx)C*q=O;#W_N}gCw_kF7jmLDI1;eE9b3m66DR9_9M^>1Yv9?g%)#D7Sr72Nm z9f=U7*X2JwB{Ix)ZQ6tzhFVoyO1PGoV7&~#AIaXD%F-@GMZPJ4?^O{PW?XxXod!TB zuTOa2+$o~p;G^9{w`W&?iLRfjQ0sz`bS`m|P6RSb$-Y1MFjc5|d(5UlVGapiUxTGh zj`So+`!SGaXA7a4;uP#o0d-emj@rc%9>T5`+0gA~7V^~6icmin!wh<7J36*J_HtLT zaggod46wSnYB+S4>XXcQA&C0-gZMU0Lf8tiS$17TytRSv?NCW)Lv~yFlL2mip$B`x z7EcU9*kV$e!*^EtPw0V!9dZ5CN}}hfo;$C1#~@Aeoxk^Q-M2o!u8$+FIxBS@nX_<| z=dChP{~F0Hs?k8m6z7gsno!t?i;CSYI(SLQdKAzuAF^o1ok5&G#N{J9Q ziuXN}U1~@3VP4&^tc@XSC5?h$$fG0YRuDziEYWbOko`8gC@h}#_KRGJ#Z&l7j?QEC z{;=NAw9Y!4uj9B+IpBJsDYf!nC&o18?bOe_GLNCc^}rwjg2&;knzpgW z=QTL$rv0HnQC0hJV%;VzOIv5;r#)k$oC8HRH^2b`H2+`?-iJK$1<%UTOhic=7sI&= zTgpol#VPVcF~M%CrtWLP>vRysxtNVF#z9)4XI7YbF%cGTX-%?7LmOl~o7n1=`YHuj z(rsrZZBOosLGv~Ay+R(x88|hWG!mOt+q~7!m{@aGg6{YoGf8-cq{HNU#`cTUiq%18{cr>x_vn6)QfbIkX0GkDYnNOgU|*Jc zY5V5KA)=3qAZ;N@@@g1qFZ7_FH9p3GCC2`kFMj*24oP8wIgy~Ws=}QU+yd&wd_tCN zTALM7hPd@cn8My6!#%xdXi zO~%FqVfB7@>#lGpK-geYS-BTkUXn;&$-7stUN@Q(s_&)GschQER8p@J{Z`yvKR+#N z$#hnny)V<)(SF2~V&nkw4Q)H!S3y8U4*S4vJX_DNGVAq%Ou8YquNtM3?>k}X7-4y- zLe|3k$I+22vWDMvBrLw4weC)T-~r1}KKoLUP#Uf}p?pb3-?}9@f|25>GGVYaF#-uT zl)MlYypXQOy%77V%4YY2EDHAKS4sW-;z-~c4IM}4-F6>x>Y>(R=7!+rnMn9elab28 zq2d@LiVXas$E+o32XU=R|ZP-?9?sLef#m5jJ%Xgplvtl-q}VJF2Js) zj@BH-$8Plmv6t5z4}2~jH=vEH{kdq;NP#hIlgzhqZ}<|!VEPJjO6k-x63iMI1D!2% zxa@awHF+-|$|diF$2>R{QPUIr9#a4L+sEeP=ezeNbgnUK_ z?-$5n^D7)-bz2&%-WWB>a!{y}E0iSEoY|m?&r_M4^z3(#*jxl{2L=vFuJ5O1YI=bbt}HsBW6NjT?I~weLukz;+nPm z)E(R5<)!G-Y3$AAv{)RP79}g$;pPEKME88x_14O!#M<|DtOlEbT{<;qQb|`XkBHV4 zC@p%7eI?`B0$~=_Cegg=&d=?dw7-{8lbI*ei6|f%7VV3P1FQ%0ibuqkLIsW;vBn2O zcUWZw?apk*MhDs>#brGw6fJbIS!M0?v7yl`16c8N0RzLQ7Df-W9-51+Hx3dpkuz@| z=njE1=nY|GHBmH>MHYyHo!oV0)OunRlgK?N?TY^@O>tqiI^*CX1C)h(k03UuSZ_Fa zVi~1&61dOvAvHEmwz1VP+bK_0kv)N7U$zmszY=4gmUJ%e6q73EfSGqhZM{NaZ6q)4 zti~cQ+?g_`bk^rdf)fg%=}gR@v5;+Sa_F#?(NapW3@yzs#ltUj&Q-rxEK-n4bWRNmERVu$G?kj z>gyNrB|^r!k@z1xz? z?E3ynE~sj$T-FUV>*dVU>B&%70?RdTF5&XUD{7{JWhm1?t)AbIn?MP4?9yE@g2kU6 z^1)gf%zfL2(^*OlnePrY*{z}M&Yx#DS`MQ(L2}^M_zsbrOPI`lwYZ9B%`@JAU)keD z!seV@>cmX}z~FbKaR$B**V1Mo@t&OC*4Qmg8(C0G@nGgW3p^5OvoFouwwm}18X+7q=3h+RYR`5@zq`5ja@mTJ34y2tUGLhdAXa( z{roF4>vE~IhAaMK`)Su@Y0*P8n}9(_reLV>>jGaS0%AM$q*%E}q=JFi+mMOdj8<8_ zCpPj#uLKmmX!OR-U5^MCQp^q_&I9gexWK_{XD0AZ7u3)6KX1o+$%hF%#0>iRAt-|X z=cf~qajJ+2Skcm06vHuK4`?%Bz?LoGDao)MdBDR5AMB%ufKSP+(j&>sh;+V*u@DXi zdV4se3(x-M4mg#(@e($VO1W8xI2R-5z2tIN~u?0yybt471LEV$pkjYJ9Px{D8QMYCK)JJ&xJoAuRcd|>hb&5LB0FJ zUgh`&yuUNXUZn-S65aL$KzgUqdTX9(HCa~vEkPq!V-Cmba&!k1E)8|~Wgy@c9rV#q z5t_kYF%UL5T?*!O0Wba*Uv-7dc$Hd2#I9?C@nj1trz}ZjCOQWO+19aXApSiv*9S)& zgkJ&8+iE_*`Q7 zi^t;~rCxelT`2kCaWmWVQM!QhK4|{;7F=s)&%L+fag>wqI_7mJyLRvodq$UVFUI8~ z2_<)7ut}RNzhYUXU#mV#JvV0}kr!{{PQItk8jC_NLPmr0?=kBnop4clkLX`53h$p< X)L+FJr2_poS`