From 948f5fe3345cdf0816e3b11cc2e51f0c8a75e015 Mon Sep 17 00:00:00 2001 From: kr1xh Date: Fri, 13 Nov 2026 14:27:48 +0000 Subject: [PATCH] [ntuple] Support strings longer than 254 chars in RMiniFile RMiniFile writes and reads the class name, object name and title of TFile keys as TString, but it only implemented the short form with a one byte length. A string of 255 or more characters, e.g. a long RNTuple name or a long title of a TFile, aborted on an R__ASSERT. Implement the long form as TBufferFile::WriteTString does: the byte 255 followed by the length as a 32-bit big-endian integer. RTFString now owns its serialized bytes, and a new helper reads both forms. A key header that does not fit the 16-bit TKey key length now throws. Fixes https://github.com/root-project/root/issues/14460 Assisted-by: claude-code:claude-opus-5-5 --- tree/ntuple/src/RMiniFile.cxx | 135 +++++++++++++++------------ tree/ntuple/test/ntuple_minifile.cxx | 126 ++++++++++++++++++++++++- 2 files changed, 200 insertions(+), 61 deletions(-) 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");