[tree] fix handling of corrupt zip buffer in TTreeCacheUnzip

Author: jblomer

io/io/test/ZipHeader.cxx

@@ -10,6 +10,7 @@
 #include <TMemFile.h>
 #include <TNamed.h>
 #include <TTree.h>
+#include <TTreeCacheUnzip.h>
 
 #include <cstring>
 #include <memory>
@@ -165,7 +166,7 @@ TEST(RZip, CorruptHeaderTKey)
    EXPECT_TRUE(verifyFile.Get<TNamed>("tnamed"));
 }
 
-TEST(RZip, CorruptHeaderTBasket)
+TEST(RZip, CorruptHeaderTTree)
 {
    TMemFile writableFile("memfile.root", "RECREATE");
    writableFile.SetCompressionSettings(101);
@@ -178,13 +179,15 @@ TEST(RZip, CorruptHeaderTBasket)
 
    auto keysInfo = writableFile.WalkTKeys();
    std::size_t posTBasket = 0;
+   std::size_t keylenTBasket = 0;
    for (const auto &ki : keysInfo) {
       if (ki.fClassName != "TBasket")
          continue;
 
       EXPECT_EQ(0u, posTBasket); // We expect only one basket
       EXPECT_LT(ki.fLen, ki.fObjLen); // ensure it's compressed
       posTBasket = ki.fSeekKey + ki.fKeyLen;
+      keylenTBasket = ki.fKeyLen;
    }
    EXPECT_GT(posTBasket, 0);
 
@@ -207,6 +210,27 @@ TEST(RZip, CorruptHeaderTBasket)
       buffer[posTBasket + headerOffset]--;
    }
 
+   {
+      TMemFile verifyFile("memfile.root", TMemFile::ZeroCopyView_t(buffer.get(), writableFile.GetSize()));
+
+      tree = verifyFile.Get<TTree>("t");
+
+      TTreeCacheUnzip cache(tree);
+      char *dest = nullptr;
+
+      for (int headerOffset : {3, 6}) {
+         ROOT::TestSupport::CheckDiagsRAII checkDiag;
+         checkDiag.requiredDiag(kError, "TTreeCacheUnzip::UnzipBuffer", "nbytes", /* matchFullMessage= */ false);
+
+         buffer[posTBasket + headerOffset]++;
+         EXPECT_EQ(-1, cache.UnzipBuffer(&dest, &buffer[posTBasket - keylenTBasket]));
+         buffer[posTBasket + headerOffset]--;
+      }
+
+      EXPECT_GT(cache.UnzipBuffer(&dest, &buffer[posTBasket - keylenTBasket]), 0);
+      delete[] dest;
+   }
+
    TMemFile verifyFile("memfile.root", TMemFile::ZeroCopyView_t(buffer.get(), writableFile.GetSize()));
    tree = verifyFile.Get<TTree>("t");
    tree->SetBranchAddress("val", &val);

tree/tree/src/TTreeCacheUnzip.cxx

@@ -858,11 +858,15 @@ Int_t TTreeCacheUnzip::UnzipBuffer(char **dest, char *src)
    Int_t nbytes = 0, objlen = 0, keylen = 0;
    GetRecordHeader(src, hlen, nbytes, objlen, keylen);
 
+   Int_t nbytesRemain = nbytes - keylen;
+   Int_t objlenRemain = objlen;
+
    if (!(*dest)) {
       /* early consistency check */
       UChar_t *bufcur = (UChar_t *) (src + keylen);
       Int_t nin, nbuf;
-      if(objlen > nbytes - keylen && R__unzip_header(&nin, bufcur, &nbuf) != 0) {
+      if((objlen > nbytes - keylen) &&
+         ((nbytesRemain < ROOT::Internal::kZipHeaderSize) || (R__unzip_header(&nin, bufcur, &nbuf) != 0))) {
          Error("UnzipBuffer", "Inconsistency found in header (nin=%d, nbuf=%d)", nin, nbuf);
          uzlen = -1;
          return uzlen;
@@ -893,9 +897,10 @@ Int_t TTreeCacheUnzip::UnzipBuffer(char **dest, char *src)
       Int_t nout = 0;
       Int_t noutot = 0;
 
-      while (true) {
+      while (nbytesRemain >= ROOT::Internal::kZipHeaderSize) {
          Int_t hc = R__unzip_header(&nin, bufcur, &nbuf);
-         if (hc != 0) break;
+         if ((hc != 0) || (nin > nbytesRemain) || (nbuf > objlenRemain))
+            break;
          if (gDebug > 2)
             Info("UnzipBuffer", " nin:%d, nbuf:%d, bufcur[3] :%d, bufcur[4] :%d, bufcur[5] :%d ",
                  nin, nbuf, bufcur[3], bufcur[4], bufcur[5]);
@@ -920,6 +925,8 @@ Int_t TTreeCacheUnzip::UnzipBuffer(char **dest, char *src)
          if (noutot >= objlen) break;
          bufcur += nin;
          objbuf += nout;
+         nbytesRemain -= nin;
+         objlenRemain -= nout;
       }
 
       if (noutot != objlen) {