[io] Add buffer bound checks in TFile::Recover

core/base/inc/TString.h

@@ -305,6 +305,8 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) {
    virtual void     ReadBuffer(char *&buffer);
    virtual Int_t    Sizeof() const;
 
+   std::size_t ReadBuffer(char *&buffer, std::size_t bufsize);
+
    static TString  *ReadString(TBuffer &b, const TClass *clReq);
    static void      WriteString(TBuffer &b, const TString *a);
 

core/base/src/TString.cxx

@@ -1355,7 +1355,73 @@ void TString::ReadBuffer(char *&buffer)
 
    char *data = Init(nchars, nchars);
 
-   for (int i = 0; i < nchars; i++) frombuf(buffer, &data[i]);
+   memcpy(data, buffer, nchars);
+   buffer += nchars;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// Safer version of ReadBuffer(char *&buffer), doing bound checks on the given buffer.
+/// This overload should be preferred over the other, which should be considered unsafe.
+/// \return The amount of bytes read from the buffer, or 0 in case of errors.
+
+std::size_t TString::ReadBuffer(char *&buffer, std::size_t bufsize)
+{
+   // NOTE: this is not a lambda because we want [[nodiscard]].
+   struct {
+      TString *fOuter;
+      std::size_t fRemainingBufSize;
+
+      [[nodiscard]] bool operator()(std::size_t additionalBytesNeeded)
+      {
+         if (R__unlikely(additionalBytesNeeded > fRemainingBufSize)) {
+            Error("TString::ReadBuffer", "given buffer is too small (%zu B remaining, need at least %zu more)",
+                  fRemainingBufSize, additionalBytesNeeded);
+            fOuter->UnLink();
+            fOuter->Zero();
+            return false;
+         }
+         fRemainingBufSize -= additionalBytesNeeded;
+         return true;
+      }
+   } RequireAdditionalBufCapacity{this, bufsize};
+
+   if (!RequireAdditionalBufCapacity(1)) {
+      return 0;
+   }
+
+   UnLink();
+   Zero();
+
+   UChar_t strLength;
+   Int_t   nchars;
+
+   // frombuf needs a non-const buffer, although it actually doesn't modify it.
+   char *buf = const_cast<char *>(buffer);
+   frombuf(buf, &strLength);
+   if (strLength == 255) {
+      if (!RequireAdditionalBufCapacity(sizeof(nchars))) {
+         return 0;
+      }
+      frombuf(buf, &nchars);
+   } else {
+      nchars = strLength;
+   }
+
+   if (nchars < 0) {
+      Error("TString::ReadBuffer", "found case with nwh=%d and nchars=%d", strLength, nchars);
+      return 0;
+   }
+
+   char *data = Init(nchars, nchars);
+
+   if (!RequireAdditionalBufCapacity(nchars)) {
+      return 0;
+   }
+   memcpy(data, buf, nchars);
+
+   std::size_t nbytesRead = bufsize - RequireAdditionalBufCapacity.fRemainingBufSize;
+   buffer += nbytesRead;
+   return nbytesRead;
 }
 
 ////////////////////////////////////////////////////////////////////////////////

io/io/inc/TKey.h

@@ -108,8 +108,13 @@ class TKey : public TNamed {
    virtual void       *ReadObjectAny(const TClass *expectedClass);
    virtual void        ReadBuffer(char *&buffer);
            void        ReadKeyBuffer(char *&buffer);
-   virtual Bool_t      ReadFile();
-   virtual void        SetBuffer() { DeleteBuffer(); fBuffer = new char[fNbytes];}
+           bool ReadKeyBuffer(char *&buffer, std::size_t bufsize);
+           virtual Bool_t ReadFile();
+           virtual void SetBuffer()
+           {
+              DeleteBuffer();
+              fBuffer = new char[fNbytes];
+           }
    virtual void        SetParent(const TObject *parent);
            void        SetMotherDir(TDirectory* dir) { fMotherDir = dir; }
            Int_t       Sizeof() const override;

io/io/src/TFile.cxx

@@ -1396,8 +1396,8 @@ TFile::InfoListRet TFile::GetStreamerInfoListImpl(bool lookupSICache)
    if (fSeekInfo) {
       TDirectory::TContext ctxt(this); // gFile and gDirectory used in ReadObj
       auto key = std::make_unique<TKey>(this);
-      std::vector<char> buffer(fNbytesInfo+1);
-      auto buf = buffer.data();
+      auto buffer = std::make_unique<char[]>(fNbytesInfo+1);
+      auto buf = buffer.get();
       Seek(fSeekInfo);                         // NOLINT: silence clang-tidy warnings
       if (ReadBuffer(buf,fNbytesInfo)) {       // NOLINT: silence clang-tidy warnings
          // ReadBuffer returns kTRUE in case of failure.
@@ -1419,8 +1419,9 @@ TFile::InfoListRet TFile::GetStreamerInfoListImpl(bool lookupSICache)
             return {nullptr, 0, hash};
          }
       }
-      key->ReadKeyBuffer(buf);
-      list = dynamic_cast<TList*>(key->ReadObjWithBuffer(buffer.data()));
+      if (!key->ReadKeyBuffer(buf, fNbytesInfo))
+         return {nullptr, 1, hash};
+      list = dynamic_cast<TList*>(key->ReadObjWithBuffer(buffer.get()));
       if (list) list->SetOwner();
    } else {
       list = (TList*)Get("StreamerInfo"); //for versions 2.26 (never released)
@@ -2100,13 +2101,6 @@ TProcessID  *TFile::ReadProcessID(UShort_t pidf)
 
 Int_t TFile::Recover()
 {
-   Short_t  keylen,cycle;
-   UInt_t   datime;
-   Int_t    nbytes,date,time,objlen,nwheader;
-   Long64_t seekkey,seekpdir;
-   char     header[1024];
-   char    *buffer, *bufread;
-   char     nwhc;
    Long64_t idcur = fBEGIN;
 
    Long64_t size;
@@ -2120,10 +2114,11 @@ Int_t TFile::Recover()
    if (fWritable && !fFree) fFree  = new TList;
 
    Int_t nrecov = 0;
-   nwheader = 1024;
-   Int_t nread = nwheader;
 
    while (idcur < fEND) {
+      char header[1024];
+      int nread = sizeof(header);
+
       Seek(idcur);                             // NOLINT: silence clang-tidy warnings
       if (idcur+nread >= fEND) nread = fEND-idcur-1;
       if (ReadBuffer(header, nread)) {         // NOLINT: silence clang-tidy warnings
@@ -2132,8 +2127,8 @@ Int_t TFile::Recover()
                GetName(),idcur);
          break;
       }
-      buffer  = header;
-      bufread = header;
+      char *buffer = header;
+      Int_t nbytes;
       frombuf(buffer, &nbytes);
       if (!nbytes) {
          Error("Recover","Address = %lld\tNbytes = %d\t=====E R R O R=======", idcur, nbytes);
@@ -2147,10 +2142,15 @@ Int_t TFile::Recover()
       }
       Version_t versionkey;
       frombuf(buffer, &versionkey);
+      Int_t objlen;
       frombuf(buffer, &objlen);
+      UInt_t datime;
       frombuf(buffer, &datime);
+      Short_t keylen;
       frombuf(buffer, &keylen);
+      Short_t cycle;
       frombuf(buffer, &cycle);
+      Long64_t seekkey, seekpdir;
       if (versionkey > 1000) {
          frombuf(buffer, &seekkey);
          frombuf(buffer, &seekpdir);
@@ -2159,20 +2159,23 @@ Int_t TFile::Recover()
          frombuf(buffer, &skey);  seekkey  = (Long64_t)skey;
          frombuf(buffer, &sdir);  seekpdir = (Long64_t)sdir;
       }
-      frombuf(buffer, &nwhc);
-      char *classname = nullptr;
-      if (nwhc <= 0 || nwhc > 100) break;
-      classname = new char[nwhc+1];
-      int i, nwhci = nwhc;
-      for (i = 0;i < nwhc; i++) frombuf(buffer, &classname[i]);
-      classname[nwhci] = '\0';
+      char classnameLen;
+      frombuf(buffer, &classnameLen);
+      char classname[101];
+      if (classnameLen <= 0 || classnameLen > (Int_t)sizeof(classname))
+         break;
+      memcpy(classname, buffer, classnameLen);
+      buffer += classnameLen;
+      classname[static_cast<std::size_t>(classnameLen)] = '\0';
+      Int_t date, time;
       TDatime::GetDateTime(datime, date, time);
       TClass *tclass = TClass::GetClass(classname);
       if (seekpdir == fSeekDir && tclass && !tclass->InheritsFrom(TFile::Class())
                                && strcmp(classname,"TBasket")) {
          TKey *key = new TKey(this);
-         key->ReadKeyBuffer(bufread);
-         if (!strcmp(key->GetName(),"StreamerInfo")) {
+         char *bufread = header;
+         bool keyRead = key->ReadKeyBuffer(bufread, sizeof(header));
+         if (!keyRead || !strcmp(key->GetName(), "StreamerInfo")) {
             fSeekInfo = seekkey;
             SafeDelete(fInfoCache);
             fNbytesInfo = nbytes;
@@ -2184,7 +2187,6 @@ Int_t TFile::Recover()
             Info("Recover", "%s, recovered key %s:%s at address %lld",GetName(),key->GetClassName(),key->GetName(),idcur);
          }
       }
-      delete [] classname;
       idcur += nbytes;
    }
    if (fWritable) {

io/io/src/TKey.cxx

@@ -1284,6 +1284,132 @@ void TKey::ReadKeyBuffer(char *&buffer)
    fTitle.ReadBuffer(buffer);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+/// Decode input buffer.
+/// \return true if decoding was successful.
+
+bool TKey::ReadKeyBuffer(char *&buffer, std::size_t bufsize)
+{
+   // NOTE: this is not a lambda because we want [[nodiscard]].
+   struct {
+      TKey *fOuter;
+      std::size_t fRemainingBufSize;
+      [[nodiscard]] bool operator()(std::size_t additionalBytesNeeded) {
+         if (R__unlikely(additionalBytesNeeded > fRemainingBufSize)) {
+            fOuter->Error("ReadKeyBuffer", "The given buffer is too small to fit this TKey.");
+            fOuter->MakeZombie();
+            return false;
+         }
+         fRemainingBufSize -= additionalBytesNeeded;
+         return true;
+      }
+   } RequireAdditionalBufCapacity{this, bufsize};
+
+   // Min size of the buffer for reading the common key header data
+   constexpr std::size_t kMinBufSize =
+      sizeof(fNbytes) + sizeof(Version_t) + sizeof(fObjlen) + sizeof(fKeylen) + sizeof(fCycle);
+   if (!RequireAdditionalBufCapacity(kMinBufSize))
+      return false;
+
+   frombuf(buffer, &fNbytes);
+   if (fNbytes < 0) {
+      Error("ReadKeyBuffer", "The value of fNbytes is negative (%d): cannot continue to read the key buffer.", fNbytes);
+      MakeZombie();
+      fNbytes = 0;
+      return false;
+   }
+   Version_t version;
+   frombuf(buffer,&version);
+   fVersion = (Int_t)version;
+   frombuf(buffer, &fObjlen);
+   if (fObjlen < 0) {
+      Error("ReadKeyBuffer", "The value of fObjlen is negative (%d): cannot continue to read the key buffer.", fObjlen);
+      MakeZombie();
+      fObjlen = 0;
+      return false;
+   }
+   fDatime.ReadBuffer(buffer);
+   frombuf(buffer, &fKeylen);
+   if (fKeylen < 0) {
+      Error("ReadKeyBuffer", "The value of fKeylen is negative (%d): cannot continue to read the key buffer.", fKeylen);
+      MakeZombie();
+      fKeylen = 0;
+      return false;
+   }
+
+   if (fNbytes < fKeylen) {
+      Error("ReadKeyBuffer", "fNbytes (%d) < fKeylen (%d): cannot continue to read the key buffer.", fNbytes, fKeylen);
+      MakeZombie();
+      return false;
+   }
+
+   constexpr auto maxInt_t = std::numeric_limits<Int_t>::max();
+   if (fKeylen > (maxInt_t - fObjlen)) {
+      Error("ReadKeyBuffer", "fObjlen (%d) + fKeylen (%d) > max int (%d): cannot continue to read the key buffer.", fObjlen, fKeylen, maxInt_t);
+      MakeZombie();
+      return false;
+   }
+
+   frombuf(buffer, &fCycle);
+   // The initial bufsize check guarantees that we could read up to here.
+   // From now on we need to be careful.
+
+   if (fVersion > 1000) {
+      if (!RequireAdditionalBufCapacity(sizeof(fSeekKey) + sizeof(Long64_t)))
+         return false;
+
+      frombuf(buffer, &fSeekKey);
+
+      // We currently store in the 16 highest bit of fSeekPdir the value of
+      // fPidOffset.  This offset is used when a key (or basket) is transferred from one
+      // file to the other.  In this case the TRef and TObject might have stored a
+      // pid index (to retrieve TProcessIDs) which refered to their order on the original
+      // file, the fPidOffset is to be added to those values to correctly find the
+      // TProcessID.  This fPidOffset needs to be increment if the key/basket is copied
+      // and need to be zero for new key/basket.
+      Long64_t pdir;
+      frombuf(buffer, &pdir);
+      fPidOffset = pdir >> kPidOffsetShift;
+      fSeekPdir = pdir & kPidOffsetMask;
+   } else {
+      if (!RequireAdditionalBufCapacity(2 * sizeof(UInt_t)))
+         return false;
+
+      UInt_t seekkey,seekdir;
+      frombuf(buffer, &seekkey);
+      fSeekKey = (Long64_t)seekkey;
+      frombuf(buffer, &seekdir);
+      fSeekPdir = (Long64_t)seekdir;
+   }
+
+   auto nRead = fClassName.ReadBuffer(buffer, RequireAdditionalBufCapacity.fRemainingBufSize);
+   if (!nRead) {
+      MakeZombie();
+      return false;
+   }
+   RequireAdditionalBufCapacity.fRemainingBufSize -= nRead;
+
+   //the following test required for forward and backward compatibility
+   if (fClassName == "TDirectory") {
+      fClassName = "TDirectoryFile";
+      SetBit(kIsDirectoryFile);
+   }
+
+   nRead = fName.ReadBuffer(buffer, RequireAdditionalBufCapacity.fRemainingBufSize);
+   if (!nRead) {
+      MakeZombie();
+      return false;
+   }
+   RequireAdditionalBufCapacity.fRemainingBufSize -= nRead;
+
+   nRead = fTitle.ReadBuffer(buffer, RequireAdditionalBufCapacity.fRemainingBufSize);
+   if (!nRead) {
+      MakeZombie();
+      return false;
+   }
+   return true;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 /// Read the key structure from the file
 

roottest/root/io/recover/CMakeLists.txt

@@ -6,3 +6,13 @@ ROOTTEST_ADD_TEST(zombie
 ROOTTEST_ADD_TEST(empty
                   MACRO runempty.C
                   OUTREF empty.ref)
+
+# corrupted_recover.root contains an otherwise-valid ROOT file that has been tweaked in the following way:
+# - fEND is changed to a larger size than the file, to trigger the recover;
+# - a fake key has been appended, that has a className "TH1F" and an object name whose string length is 2 GiB long.
+# The test verifies that Recover doesn't try to read out of bounds and refuses to read the string, while still
+# properly recovering the first two valid keys.
+ROOTTEST_ADD_TEST(corrupted_objname
+                  COPY_TO_BUILDDIR corrupted_recover.root
+                  COMMAND ${ROOT_root_CMD} -q corrupted_recover.root
+                  PASSREGEX "given buffer is too small \\(992 B remaining, need at least 2147483646 more\\)")