diff --git a/tree/ntuple/src/RMiniFile.cxx b/tree/ntuple/src/RMiniFile.cxx index 243f939b4bf57..72b4375558007 100644 --- a/tree/ntuple/src/RMiniFile.cxx +++ b/tree/ntuple/src/RMiniFile.cxx @@ -823,19 +823,26 @@ ROOT::RResult ROOT::Internal::RMiniFileReader::GetNTupleProperAtO std::uint64_t compSize, std::uint64_t uncompLen) { + if (compSize > uncompLen) { + return R__FAIL("Invalid size and length combination of anchor at offset " + std::to_string(payloadOffset) + + " (length: " + std::to_string(uncompLen) + ", bytes on disk: " + std::to_string(compSize) + ")"); + } + // The object length can be smaller than the size of RTFNTuple if it comes from a past RNTuple class version, // or larger than it if it comes from a future RNTuple class version. auto bufAnchor = MakeUninitArray(std::max(uncompLen, sizeof(RTFNTuple))); RTFNTuple *ntuple = new (bufAnchor.get()) RTFNTuple; - if (compSize != uncompLen) { + if (compSize < uncompLen) { // Read into a temporary buffer - auto unzipBuf = MakeUninitArray(std::max(uncompLen, sizeof(RTFNTuple))); - ReadBuffer(unzipBuf.get(), compSize, payloadOffset); + auto zipBuf = MakeUninitArray(compSize); + ReadBuffer(zipBuf.get(), compSize, payloadOffset); // Unzip into the final buffer - RNTupleDecompressor::Unzip(unzipBuf.get(), compSize, uncompLen, ntuple); + RNTupleDecompressor::Unzip(zipBuf.get(), compSize, uncompLen, ntuple); } else { - ReadBuffer(ntuple, compSize, payloadOffset); + // We can directly decompress into ntuple because this RTFNTuple object has been constructed in a memory area + // of (max(uncompLen, sizeof(RTFNTuple))) bytes. + ReadBuffer(ntuple, uncompLen, payloadOffset); } // We require that future class versions only append members and store the checksum in the last 8 bytes diff --git a/tree/ntuple/test/ntuple_minifile.cxx b/tree/ntuple/test/ntuple_minifile.cxx index a93449eb69c51..955659da6542d 100644 --- a/tree/ntuple/test/ntuple_minifile.cxx +++ b/tree/ntuple/test/ntuple_minifile.cxx @@ -1,4 +1,5 @@ #include "ntuple_test.hxx" +#include #include #include #include @@ -932,3 +933,49 @@ TEST(MiniFile, FreeSlots) EXPECT_TRUE(found); } } + +TEST(MiniFile, InvalidAnchor) +{ + FileRaii fileGuard("test_ntuple_minifile_invalid_anchor.root"); + RNTupleWriteOptions options; + auto writer = RNTupleFileWriter::Recreate("ntpl", fileGuard.GetPath(), EContainerFormat::kTFile, options); + writer->Commit(); + + auto tfile = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str())); + ASSERT_TRUE(tfile && !tfile->IsZombie()); + Long64_t seekKey = 0; + int objLen = 0; + int nbytes = 0; + auto keys = tfile->WalkTKeys(); + for (const auto &k : keys) { + if (k.fKeyName == "ntpl") { + seekKey = k.fSeekKey; + objLen = k.fObjLen; + ASSERT_GT(k.fLen, k.fKeyLen); + nbytes = k.fLen - k.fKeyLen; + break; + } + } + ASSERT_GT(seekKey, 0); + ASSERT_GE(objLen, nbytes); + + // We corrupt the RNTuple anchor key such that the size on disk becomes _larger_ than the length of the object. + auto fp = fopen(fileGuard.GetPath().c_str(), "r+b"); + ASSERT_TRUE(fp != nullptr); + ASSERT_EQ(0, fseek(fp, seekKey, SEEK_SET)); + std::int32_t oversizedLen = objLen - nbytes + 1; +#ifdef R__BYTESWAP + std::int32_t onDiskVal = RByteSwap<4>::bswap(oversizedLen); + ASSERT_EQ(1u, fwrite(&onDiskVal, sizeof(std::int32_t), 1, fp)); +#else + ASSERT_EQ(1u, fwrite(&oversizedLen, sizeof(std::int32_t), 1, fp)); +#endif + fclose(fp); + + try { + RNTupleReader::Open("ntpl", fileGuard.GetPath()); + FAIL() << "invalid anchor key should fail"; + } catch (const ROOT::RException &e) { + EXPECT_THAT(e.what(), testing::HasSubstr("Invalid size and length combination of anchor")); + } +}