From b9a597c9a37092ac89dd9a234c3fa4d723604a60 Mon Sep 17 00:00:00 2001 From: silverweed Date: Fri, 8 May 2026 15:07:37 +0200 Subject: [PATCH 1/5] [core] Introduce safer overload of TString::ReadBuffer --- core/base/inc/TString.h | 2 ++ core/base/src/TString.cxx | 68 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index bdb656be39ff8..57ff86526facc 100644 --- a/core/base/inc/TString.h +++ b/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); diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index 1dfde2b7ac5b0..e89eceb012ef2 100644 --- a/core/base/src/TString.cxx +++ b/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(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; } //////////////////////////////////////////////////////////////////////////////// From 649f1ab1bb3fda09c06690a7b6c7200737b72439 Mon Sep 17 00:00:00 2001 From: silverweed Date: Fri, 8 May 2026 15:07:59 +0200 Subject: [PATCH 2/5] [io] Introduce safer overload of TKey::ReadKeyBuffer --- io/io/inc/TKey.h | 9 +++- io/io/src/TKey.cxx | 126 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/io/io/inc/TKey.h b/io/io/inc/TKey.h index 75f27524ead31..3a94c28788464 100644 --- a/io/io/inc/TKey.h +++ b/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; diff --git a/io/io/src/TKey.cxx b/io/io/src/TKey.cxx index c59d0f71c8904..b523137659f0a 100644 --- a/io/io/src/TKey.cxx +++ b/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::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 From 525b3988a0c944e0774bbfbc808b7ea5a159d8d6 Mon Sep 17 00:00:00 2001 From: silverweed Date: Fri, 8 May 2026 15:08:12 +0200 Subject: [PATCH 3/5] [io] use safer overload of TKey::ReadKeyBuffer() in TFile::Recover() This prevents potential oob stack reads in case of corrupted TFiles --- io/io/src/TFile.cxx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index ce56d801d4e6f..827108b2f9df1 100644 --- a/io/io/src/TFile.cxx +++ b/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(this); - std::vector buffer(fNbytesInfo+1); - auto buf = buffer.data(); + auto buffer = std::make_unique(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(key->ReadObjWithBuffer(buffer.data())); + if (!key->ReadKeyBuffer(buf, fNbytesInfo)) + return {nullptr, 1, hash}; + list = dynamic_cast(key->ReadObjWithBuffer(buffer.get())); if (list) list->SetOwner(); } else { list = (TList*)Get("StreamerInfo"); //for versions 2.26 (never released) @@ -2171,8 +2172,9 @@ Int_t TFile::Recover() 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; From 9c222b190ce0fc59879cc611b3392896d0656888 Mon Sep 17 00:00:00 2001 From: silverweed Date: Fri, 8 May 2026 15:36:32 +0200 Subject: [PATCH 4/5] [io] Cleanup a bit TFile::Recover - Declare variables when they're used - Spare a needless dynamic allocation - Use memcpy instead of frombuf in a loop --- io/io/src/TFile.cxx | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 827108b2f9df1..9e615e60f6425 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -2101,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; @@ -2121,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 @@ -2133,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); @@ -2148,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); @@ -2160,13 +2159,15 @@ 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(classnameLen)] = '\0'; + Int_t date, time; TDatime::GetDateTime(datime, date, time); TClass *tclass = TClass::GetClass(classname); if (seekpdir == fSeekDir && tclass && !tclass->InheritsFrom(TFile::Class()) @@ -2186,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) { From 5aa5f114421237a583ce85fa04908aeed30b935f Mon Sep 17 00:00:00 2001 From: silverweed Date: Mon, 11 May 2026 11:31:06 +0200 Subject: [PATCH 5/5] [io] add integration test for TFile recover --- roottest/root/io/recover/CMakeLists.txt | 10 ++++++++++ roottest/root/io/recover/corrupted_recover.root | Bin 0 -> 2048 bytes 2 files changed, 10 insertions(+) create mode 100644 roottest/root/io/recover/corrupted_recover.root diff --git a/roottest/root/io/recover/CMakeLists.txt b/roottest/root/io/recover/CMakeLists.txt index 18b214391ccd6..a31bb9a9a5480 100644 --- a/roottest/root/io/recover/CMakeLists.txt +++ b/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\\)") diff --git a/roottest/root/io/recover/corrupted_recover.root b/roottest/root/io/recover/corrupted_recover.root new file mode 100644 index 0000000000000000000000000000000000000000..bbc31388e23e29a16cf174779d23b43071077c53 GIT binary patch literal 2048 zcmeHGJ!=$E6g~T;QG=2#B37nIsUeFxiy*S`BRG%{VF%I}hn<}lna%7@n0+E!C1PV^ z6SNTW4=lB|Ol55mEUYC&P!MgLoco4xG!Zn|$zHhc&fNRnJ!j5+6GxE-K5qf=fg}M+ zQ+#gH3-qYW_>I0yw@`R%Z$BPCGq$t&^7{KH>#slJYmyrG8q3~S_0kbZ$UDOeuo1k@ z-R;}3O=qG3BhI|)&IX~HY{hYFaAhv)-Lo#b`dsa*TeB|I>eJSA-LmSJr()sxce_v# z7y|`t5GA4;IH0)sn{N;%xh)#1RdfEPP%WHd(H}d&@I<@qdv4@)R9laxVL}HwR1M%H z@XmYw3Y9lU&sR@>Fye^sfA>5u(KPAp4YG+UA*Qv*2}RWD3&a5lZ{e zq!SCBUl6ag`a#32-Lq%mQ9+5(ea#-`Cksrhq{6IUc5c;X xleHxI&O%W((z5?+e@v@+{ZISX7w?E9G~&t4%uKSrcn+VHhH