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 0000000000000..c5cc790571f86 Binary files /dev/null and b/roottest/root/tree/basket/corrupted.root differ 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) {