diff --git a/tree/ntuple/src/RMiniFile.cxx b/tree/ntuple/src/RMiniFile.cxx index 72b4375558007..5627556fe495a 100644 --- a/tree/ntuple/src/RMiniFile.cxx +++ b/tree/ntuple/src/RMiniFile.cxx @@ -155,29 +155,54 @@ class RUInt64BE { } }; -#pragma pack(push, 1) -/// A name (type, identifier, ...) in the TFile binary format -struct RTFString { - unsigned char fLName{0}; - char fData[255]; - RTFString() = default; - RTFString(const std::string &str) - { - // The length of strings with 255 characters and longer are encoded with a 32-bit integer following the first - // byte. This is currently not handled. - R__ASSERT(str.length() < 255); - fLName = static_cast(str.length()); - memcpy(fData, str.data(), fLName); - } - std::size_t GetSize() const +/// A name (type, identifier, ...) in the TFile binary format, serialized as by TBufferFile::WriteTString(): +/// strings with up to 254 characters are prefixed by a one byte length. Longer strings are prefixed by the byte 255 +/// followed by the length as a 32-bit big-endian integer. +class RTFString { + std::string fSerialized; + +public: + static constexpr unsigned char kLongStringMarker = 255; + + RTFString() : fSerialized(1, '\0') {} + RTFString(std::string_view str) { - // A length of 255 is special and means that the first byte is followed by a 32-bit integer with the actual - // length. - R__ASSERT(fLName != 255); - return 1 + fLName; + if (str.length() < kLongStringMarker) { + fSerialized.push_back(static_cast(str.length())); + } else { + if (str.length() > static_cast(std::numeric_limits::max())) + throw ROOT::RException(R__FAIL("string too long for the TFile format")); + RUInt32BE length{static_cast(str.length())}; + fSerialized.push_back(static_cast(kLongStringMarker)); + fSerialized.append(reinterpret_cast(&length), sizeof(length)); + } + fSerialized.append(str); } + /// The size of the serialized string, including the length prefix + std::size_t GetSize() const { return fSerialized.size(); } + const char *GetData() const { return fSerialized.data(); } }; +/// Reads the TFile string at the given offset. Returns the string and its size on disk including the length prefix. +std::pair ReadTFString(ROOT::Internal::RMiniFileReader &reader, std::uint64_t offset) +{ + unsigned char shortLength = 0; + reader.ReadBuffer(&shortLength, 1, offset); + std::uint64_t prefixSize = 1; + std::uint64_t length = shortLength; + if (shortLength == RTFString::kLongStringMarker) { + RUInt32BE longLength; + reader.ReadBuffer(&longLength, sizeof(longLength), offset + prefixSize); + prefixSize += sizeof(longLength); + length = longLength; + } + std::string str(length, '\0'); + if (length > 0) + reader.ReadBuffer(str.data(), length, offset + prefixSize); + return {std::move(str), prefixSize + length}; +} + +#pragma pack(push, 1) /// The timestamp format used in TFile; the default constructor initializes with the current time struct RTFDatetime { RUInt32BE fDatetime; @@ -222,7 +247,11 @@ struct RTFKey { // For writing, we always produce "big" keys with 64-bit SeekKey and SeekPdir. fVersion = fVersion + kBigKeyVersion; fObjLen = szObjInMem; - fKeyLen = static_cast(GetHeaderSize() + clName.GetSize() + objName.GetSize() + titleName.GetSize()); + const auto keyLen = GetHeaderSize() + clName.GetSize() + objName.GetSize() + titleName.GetSize(); + // TKey stores the key length as a signed 16-bit integer + if (keyLen > static_cast(std::numeric_limits::max())) + throw ROOT::RException(R__FAIL("TFile key header too long: " + std::to_string(keyLen) + " bytes")); + fKeyLen = static_cast(keyLen); fInfoLong.fSeekKey = seekKey; fInfoLong.fSeekPdir = seekPdir; // Depends on fKeyLen being set @@ -697,17 +726,13 @@ std::uint64_t ROOT::Internal::RMiniFileReader::SearchInDirectory(std::uint64_t & auto offsetNextKey = offset + key.fKeyLen; offset += key.GetHeaderSize(); - RTFString name; - ReadBuffer(&name, 1, offset); - ReadBuffer(&name, name.GetSize(), offset); - if (std::string_view(name.fData, name.fLName) != typeName) { + const auto [className, classNameSize] = ReadTFString(*this, offset); + if (className != typeName) { offset = offsetNextKey; continue; } - offset += name.GetSize(); - ReadBuffer(&name, 1, offset); - ReadBuffer(&name, name.GetSize(), offset); - if (std::string_view(name.fData, name.fLName) == keyName) { + offset += classNameSize; + if (ReadTFString(*this, offset).first == keyName) { return key.GetSeekKey(); } offset = offsetNextKey; @@ -769,15 +794,12 @@ ROOT::RResult ROOT::Internal::RMiniFileReader::GetNTupleProper(st fNbytesKeyAndInfo = fileHeader.GetNbytesInfo(); RTFKey key; - RTFString name; ReadBuffer(&key, sizeof(key), fileHeader.fBEGIN); // Skip over the entire key length, including the class name, object name, and title stored in it. std::uint64_t offset = fileHeader.fBEGIN + key.fKeyLen; // Skip over the name and title of the TNamed preceding the TFile (root TDirectory) entry. - ReadBuffer(&name, 1, offset); - offset += name.GetSize(); - ReadBuffer(&name, 1, offset); - offset += name.GetSize(); + offset += ReadTFString(*this, offset).second; + offset += ReadTFString(*this, offset).second; // split ntupleName by '/' character to open datasets in subdirectories. std::string ntuplePathTail(ntuplePath); @@ -866,16 +888,13 @@ ROOT::RResult ROOT::Internal::RMiniFileReader::GetNTupleBare(std: { RBareFileHeader fileHeader; ReadBuffer(&fileHeader, sizeof(fileHeader), 0); - RTFString name; auto offset = sizeof(fileHeader); - ReadBuffer(&name, 1, offset); - ReadBuffer(&name, name.GetSize(), offset); - std::string_view foundName(name.fData, name.fLName); + const auto [foundName, foundNameSize] = ReadTFString(*this, offset); if (foundName != ntupleName) { - return R__FAIL("expected RNTuple named '" + std::string(ntupleName) + "' but instead found '" + - std::string(foundName) + "' in file '" + fRawFile->GetUrl() + "'"); + return R__FAIL("expected RNTuple named '" + std::string(ntupleName) + "' but instead found '" + foundName + + "' in file '" + fRawFile->GetUrl() + "'"); } - offset += name.GetSize(); + offset += foundNameSize; RTFNTuple ntuple; ReadBuffer(&ntuple, sizeof(ntuple), offset); @@ -972,11 +991,11 @@ void ROOT::Internal::RNTupleFileWriter::PrepareBlobKey(std::int64_t offset, size unsigned char *writeBuffer = buffer; memcpy(writeBuffer, &keyHeader, keyHeader.GetHeaderSize()); writeBuffer += keyHeader.GetHeaderSize(); - memcpy(writeBuffer, &strClass, strClass.GetSize()); + memcpy(writeBuffer, strClass.GetData(), strClass.GetSize()); writeBuffer += strClass.GetSize(); - memcpy(writeBuffer, &strObject, strObject.GetSize()); + memcpy(writeBuffer, strObject.GetData(), strObject.GetSize()); writeBuffer += strObject.GetSize(); - memcpy(writeBuffer, &strTitle, strTitle.GetSize()); + memcpy(writeBuffer, strTitle.GetData(), strTitle.GetSize()); writeBuffer += strTitle.GetSize(); R__ASSERT(writeBuffer == buffer + kBlobKeyLen); } @@ -1142,9 +1161,9 @@ ROOT::Internal::RNTupleFileWriter::RImplSimple::WriteKey(const void *buffer, std RTFKey key(shared.fKeyOffset, directoryOffset, strClass, strObject, strTitle, len, nbytes); Write(&key, key.GetHeaderSize(), shared.fKeyOffset); - Write(&strClass, strClass.GetSize()); - Write(&strObject, strObject.GetSize()); - Write(&strTitle, strTitle.GetSize()); + Write(strClass.GetData(), strClass.GetSize()); + Write(strObject.GetData(), strObject.GetSize()); + Write(strTitle.GetData(), strTitle.GetSize()); auto offsetData = shared.fFilePos; // The next key starts after the data. shared.fKeyOffset = offsetData + nbytes; @@ -1577,7 +1596,7 @@ void ROOT::Internal::RNTupleFileWriter::WriteBareFileSkeleton(int defaultCompres auto &fileSimple = std::get(fFile); fileSimple.Write(&bareHeader, sizeof(bareHeader), 0); RTFString ntupleName{fNTupleName}; - fileSimple.Write(&ntupleName, ntupleName.GetSize()); + fileSimple.Write(ntupleName.GetData(), ntupleName.GetSize()); // Write zero-initialized ntuple to reserve the space; will be overwritten on commit RTFNTuple ntupleOnDisk; @@ -1650,15 +1669,15 @@ void ROOT::Internal::RNTupleFileWriter::WriteTFileKeysList(std::uint64_t anchorS strFileName, strEmpty, keyList.GetSize() + keyRNTuple.fKeyLen); fileSimple.Write(&keyKeyList, keyKeyList.GetHeaderSize(), fileSimple.fShared->fControlBlock->fFileRecord.GetSeekKeys()); - fileSimple.Write(&strEmpty, strEmpty.GetSize()); - fileSimple.Write(&strFileName, strFileName.GetSize()); - fileSimple.Write(&strEmpty, strEmpty.GetSize()); + fileSimple.Write(strEmpty.GetData(), strEmpty.GetSize()); + fileSimple.Write(strFileName.GetData(), strFileName.GetSize()); + fileSimple.Write(strEmpty.GetData(), strEmpty.GetSize()); fileSimple.Write(&keyList, keyList.GetSize()); fileSimple.Write(&keyRNTuple, keyRNTuple.GetHeaderSize()); // Write class name, object name, and title for this key. - fileSimple.Write(&strRNTupleClass, strRNTupleClass.GetSize()); - fileSimple.Write(&strRNTupleName, strRNTupleName.GetSize()); - fileSimple.Write(&strEmpty, strEmpty.GetSize()); + fileSimple.Write(strRNTupleClass.GetData(), strRNTupleClass.GetSize()); + fileSimple.Write(strRNTupleName.GetData(), strRNTupleName.GetSize()); + fileSimple.Write(strEmpty.GetData(), strEmpty.GetSize()); fileSimple.fShared->fControlBlock->fFileRecord.fNBytesKeys = fileShared.fFilePos - fileSimple.fShared->fControlBlock->fFileRecord.GetSeekKeys(); fileShared.fKeyOffset = fileShared.fFilePos; @@ -1736,12 +1755,12 @@ void ROOT::Internal::RNTupleFileWriter::WriteTFileSkeleton(int defaultCompressio fileSimple.Write(&keyRoot, keyRoot.GetHeaderSize(), RTFHeader::kBEGIN); // Write class name, object name, and title for the TFile key. - fileSimple.Write(&strTFile, strTFile.GetSize()); - fileSimple.Write(&strFileName, strFileName.GetSize()); - fileSimple.Write(&strEmpty, strEmpty.GetSize()); + fileSimple.Write(strTFile.GetData(), strTFile.GetSize()); + fileSimple.Write(strFileName.GetData(), strFileName.GetSize()); + fileSimple.Write(strEmpty.GetData(), strEmpty.GetSize()); // Write the name and title of the TNamed preceding the TFile entry. - fileSimple.Write(&strFileName, strFileName.GetSize()); - fileSimple.Write(&strEmpty, strEmpty.GetSize()); + fileSimple.Write(strFileName.GetData(), strFileName.GetSize()); + fileSimple.Write(strEmpty.GetData(), strEmpty.GetSize()); // Will be overwritten on commit fileSimple.fShared->fControlBlock->fSeekFileRecord = fileSimple.fShared->fFilePos; fileSimple.Write(&fileSimple.fShared->fControlBlock->fFileRecord, diff --git a/tree/ntuple/test/ntuple_minifile.cxx b/tree/ntuple/test/ntuple_minifile.cxx index 955659da6542d..fb0fd944eda40 100644 --- a/tree/ntuple/test/ntuple_minifile.cxx +++ b/tree/ntuple/test/ntuple_minifile.cxx @@ -438,9 +438,9 @@ TEST(MiniFile, LongString) FileRaii fileGuard("test_ntuple_minifile_long_string.root"); static constexpr const char *LongString = - "This is a very long text with exactly 254 characters, which is the maximum that the RNTupleWriter can currently " - "store in a TFile header. For longer strings, a length of 255 is special and means that the first length byte is " - "followed by an integer length."; + "This is a very long text with exactly 254 characters, which is the longest string that a TFile key stores with " + "a one byte length prefix. For longer strings, a length of 255 is special and means that the first length byte " + "is followed by an integer length."; std::unique_ptr file(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE", LongString)); auto writer = RNTupleFileWriter::Append("ntuple", *file, RNTupleWriteOptions::kDefaultMaxKeySize, /*isAttributesRNTuple=*/false); @@ -458,6 +458,126 @@ TEST(MiniFile, LongString) EXPECT_EQ(offFooter, ntuple1.GetSeekFooter()); } +TEST(MiniFile, LongStringTitle) +{ + // Strings longer than 254 characters are stored as a 255 marker byte followed by a 32-bit length + for (std::size_t len : {255u, 256u, 2000u}) { + SCOPED_TRACE("title length " + std::to_string(len)); + FileRaii fileGuard("test_ntuple_minifile_long_string_title_" + std::to_string(len) + ".root"); + const std::string title(len, 't'); + + std::uint64_t offHeader; + std::uint64_t offFooter; + { + std::unique_ptr file(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE", title.c_str())); + auto writer = RNTupleFileWriter::Append("ntuple", *file, RNTupleWriteOptions::kDefaultMaxKeySize, + /*isAttributesRNTuple=*/false); + char header = 'h'; + char footer = 'f'; + offHeader = writer->WriteNTupleHeader(&header, 1, 1); + offFooter = writer->WriteNTupleFooter(&footer, 1, 1); + writer->Commit(); + } + + auto rawFile = RRawFile::Create(fileGuard.GetPath()); + RMiniFileReader reader(rawFile.get()); + auto ntuple = reader.GetNTuple("ntuple").Inspect(); + EXPECT_EQ(offHeader, ntuple.GetSeekHeader()); + EXPECT_EQ(offFooter, ntuple.GetSeekFooter()); + } +} + +TEST(MiniFile, LongNTupleNameRaw) +{ + FileRaii fileGuard("test_ntuple_minifile_long_ntuple_name_raw.ntuple"); + const std::string ntupleName(300, 'n'); + + RNTupleWriteOptions options; + auto writer = RNTupleFileWriter::Recreate(ntupleName, fileGuard.GetPath(), EContainerFormat::kBare, options); + char header = 'h'; + char footer = 'f'; + auto offHeader = writer->WriteNTupleHeader(&header, 1, 1); + auto offFooter = writer->WriteNTupleFooter(&footer, 1, 1); + writer->Commit(); + + auto rawFile = RRawFile::Create(fileGuard.GetPath()); + RMiniFileReader reader(rawFile.get()); + auto ntuple = reader.GetNTuple(ntupleName).Inspect(); + EXPECT_EQ(offHeader, ntuple.GetSeekHeader()); + EXPECT_EQ(offFooter, ntuple.GetSeekFooter()); +} + +TEST(MiniFile, LongNTupleNameStream) +{ + FileRaii fileGuard("test_ntuple_minifile_long_ntuple_name_stream.root"); + const std::string ntupleName(300, 'n'); + + RNTupleWriteOptions options; + auto writer = RNTupleFileWriter::Recreate(ntupleName, fileGuard.GetPath(), EContainerFormat::kTFile, options); + char header = 'h'; + char footer = 'f'; + auto offHeader = writer->WriteNTupleHeader(&header, 1, 1); + auto offFooter = writer->WriteNTupleFooter(&footer, 1, 1); + writer->Commit(); + + auto rawFile = RRawFile::Create(fileGuard.GetPath()); + RMiniFileReader reader(rawFile.get()); + auto ntuple = reader.GetNTuple(ntupleName).Inspect(); + EXPECT_EQ(offHeader, ntuple.GetSeekHeader()); + EXPECT_EQ(offFooter, ntuple.GetSeekFooter()); + + auto file = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "READ")); + ASSERT_TRUE(file); + auto anchor = std::unique_ptr(file->Get(ntupleName.c_str())); + ASSERT_TRUE(anchor); + EXPECT_TRUE(IsEqual(ntuple, RNTupleTester(*anchor).GetAnchor())); +} + +TEST(MiniFile, LongNTupleNameProper) +{ + FileRaii fileGuard("test_ntuple_minifile_long_ntuple_name_proper.root"); + const std::string ntupleName(300, 'n'); + + std::uint64_t offHeader; + std::uint64_t offFooter; + { + std::unique_ptr file(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + auto writer = RNTupleFileWriter::Append(ntupleName, *file, RNTupleWriteOptions::kDefaultMaxKeySize, + /*isAttributesRNTuple=*/false); + char header = 'h'; + char footer = 'f'; + offHeader = writer->WriteNTupleHeader(&header, 1, 1); + offFooter = writer->WriteNTupleFooter(&footer, 1, 1); + writer->Commit(); + } + + auto rawFile = RRawFile::Create(fileGuard.GetPath()); + RMiniFileReader reader(rawFile.get()); + auto ntuple = reader.GetNTuple(ntupleName).Inspect(); + EXPECT_EQ(offHeader, ntuple.GetSeekHeader()); + EXPECT_EQ(offFooter, ntuple.GetSeekFooter()); +} + +TEST(MiniFile, KeyHeaderTooLong) +{ + FileRaii fileGuard("test_ntuple_minifile_key_header_too_long.root"); + // TKey stores the key length as a signed 16-bit integer, which limits the total length of the names in a key + const std::string ntupleName(40000, 'n'); + + RNTupleWriteOptions options; + auto writer = RNTupleFileWriter::Recreate(ntupleName, fileGuard.GetPath(), EContainerFormat::kTFile, options); + char header = 'h'; + char footer = 'f'; + writer->WriteNTupleHeader(&header, 1, 1); + writer->WriteNTupleFooter(&footer, 1, 1); + try { + writer->Commit(); + FAIL() << "key headers longer than 32767 bytes should throw"; + } catch (const ROOT::RException &err) { + EXPECT_THAT(err.what(), testing::HasSubstr("TFile key header too long")); + } +} + TEST(MiniFile, MultiKeyBlob) { FileRaii fileGuard("test_ntuple_minifile_multi_key_blob.root");