Skip to content

Commit ca2300f

Browse files
silverweedsilverweed
committed
[io] Fix WalkTKeys mishandling strings longer than 127 characters
1 parent 9093b47 commit ca2300f

File tree

2 files changed

+113
-60
lines changed

2 files changed

+113
-60
lines changed

‎io/io/src/TFile.cxx

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1684,22 +1684,27 @@ std::optional<ROOT::Detail::TKeyMapNode> ROOT::Detail::TKeyMapIterable::TIterato
16841684
}
16851685

16861686
const auto readString = [&buffer, &header](bool skipCheck = false) {
1687-
char stringLen;
1688-
char str[256];
1687+
std::uint8_t stringLenShort;
1688+
std::uint32_t stringLen;
16891689
if (!skipCheck && ((buffer - header) >= headerSize)) {
16901690
stringLen = 0;
16911691
} else {
1692-
frombuf(buffer, &stringLen);
1693-
if (stringLen < 0)
1694-
stringLen = 0;
1695-
else if ((buffer - header) + stringLen > headerSize)
1692+
frombuf(buffer, &stringLenShort);
1693+
if (stringLenShort == 0xFF)
1694+
frombuf(buffer, &stringLen);
1695+
else
1696+
stringLen = stringLenShort;
1697+
1698+
if ((buffer - header) + stringLen > headerSize)
16961699
stringLen = headerSize - (buffer - header);
16971700
}
1698-
for (int i = 0; i < stringLen; ++i)
1699-
frombuf(buffer, &str[i]);
1700-
str[static_cast<int>(stringLen)] = 0;
17011701

1702-
return std::string(str, stringLen);
1702+
std::string str;
1703+
if (stringLen)
1704+
str = std::string(buffer, stringLen);
1705+
buffer += stringLen;
1706+
1707+
return str;
17031708
};
17041709

17051710
node.fClassName = readString(true);

‎io/io/test/TFileTests.cxx

Lines changed: 98 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -16,59 +16,59 @@
1616

1717
TEST(TFile, WriteObjectTObject)
1818
{
19-
auto filename{"tfile_writeobject_tobject.root"};
20-
auto tnamed_name{"mytnamed_name"};
21-
auto tnamed_title{"mytnamed_title"};
22-
23-
{
24-
TNamed mytnamed{tnamed_name, tnamed_title};
25-
TFile f{filename, "recreate"};
26-
f.WriteObject(&mytnamed, mytnamed.GetName());
27-
f.Close();
28-
}
29-
30-
TFile input{filename};
31-
auto named = input.Get<TNamed>(tnamed_name);
32-
auto keyptr = static_cast<TKey *>(input.GetListOfKeys()->At(0));
33-
34-
EXPECT_STREQ(named->GetName(), tnamed_name);
35-
EXPECT_STREQ(named->GetTitle(), tnamed_title);
36-
EXPECT_STREQ(keyptr->GetName(), tnamed_name);
37-
EXPECT_STREQ(keyptr->GetTitle(), tnamed_title);
38-
39-
input.Close();
40-
gSystem->Unlink(filename);
19+
auto filename{"tfile_writeobject_tobject.root"};
20+
auto tnamed_name{"mytnamed_name"};
21+
auto tnamed_title{"mytnamed_title"};
22+
23+
{
24+
TNamed mytnamed{tnamed_name, tnamed_title};
25+
TFile f{filename, "recreate"};
26+
f.WriteObject(&mytnamed, mytnamed.GetName());
27+
f.Close();
28+
}
29+
30+
TFile input{filename};
31+
auto named = input.Get<TNamed>(tnamed_name);
32+
auto keyptr = static_cast<TKey *>(input.GetListOfKeys()->At(0));
33+
34+
EXPECT_STREQ(named->GetName(), tnamed_name);
35+
EXPECT_STREQ(named->GetTitle(), tnamed_title);
36+
EXPECT_STREQ(keyptr->GetName(), tnamed_name);
37+
EXPECT_STREQ(keyptr->GetTitle(), tnamed_title);
38+
39+
input.Close();
40+
gSystem->Unlink(filename);
4141
}
4242

4343
TEST(TFile, WriteObjectVector)
4444
{
45-
auto filename{"tfile_writeobject_vector.root"};
46-
auto vec_name{"object name"}; // Decided arbitrarily
45+
auto filename{"tfile_writeobject_vector.root"};
46+
auto vec_name{"object name"}; // Decided arbitrarily
4747

48-
{
49-
std::vector<int> myvec{1,2,3,4,5};
50-
TFile f{filename, "recreate"};
51-
f.WriteObject(&myvec, vec_name);
52-
f.Close();
53-
}
48+
{
49+
std::vector<int> myvec{1, 2, 3, 4, 5};
50+
TFile f{filename, "recreate"};
51+
f.WriteObject(&myvec, vec_name);
52+
f.Close();
53+
}
5454

55-
TFile input{filename};
56-
auto retvecptr = input.Get<std::vector<int>>(vec_name);
57-
const auto &retvec = *retvecptr;
58-
auto retkey = static_cast<TKey *>(input.GetListOfKeys()->At(0));
55+
TFile input{filename};
56+
auto retvecptr = input.Get<std::vector<int>>(vec_name);
57+
const auto &retvec = *retvecptr;
58+
auto retkey = static_cast<TKey *>(input.GetListOfKeys()->At(0));
5959

60-
std::vector<int> expected{1,2,3,4,5};
60+
std::vector<int> expected{1, 2, 3, 4, 5};
6161

62-
ASSERT_EQ(retvec.size(), expected.size());
63-
for (std::size_t i = 0; i < retvec.size(); ++i) {
64-
EXPECT_EQ(retvec[i], expected[i]);
65-
}
62+
ASSERT_EQ(retvec.size(), expected.size());
63+
for (std::size_t i = 0; i < retvec.size(); ++i) {
64+
EXPECT_EQ(retvec[i], expected[i]);
65+
}
6666

67-
EXPECT_STREQ(retkey->GetName(), vec_name);
68-
EXPECT_STREQ(retkey->GetTitle(), ""); // Objects that don't derive from TObject have no title
67+
EXPECT_STREQ(retkey->GetName(), vec_name);
68+
EXPECT_STREQ(retkey->GetTitle(), ""); // Objects that don't derive from TObject have no title
6969

70-
input.Close();
71-
gSystem->Unlink(filename);
70+
input.Close();
71+
gSystem->Unlink(filename);
7272
}
7373

7474
// Tests ROOT-9857
@@ -142,22 +142,22 @@ TEST(TFile, ReadWithoutGlobalRegistrationNet)
142142
TestReadWithoutGlobalRegistrationIfPossible(netFile);
143143
}
144144
#endif
145-
#endif
145+
#endif
146146

147147
// https://github.com/root-project/root/issues/16189
148148
TEST(TFile, k630forwardCompatibility)
149149
{
150150
gEnv->SetValue("TFile.v630forwardCompatibility", 1);
151151
const std::string filename{"filek30.root"};
152152
// Testing that the flag is also set when creating the file from scratch (as opposed to "UPDATE")
153-
TFile filec{filename.c_str(),"RECREATE"};
154-
ASSERT_EQ(filec.TestBit(TFile::k630forwardCompatibility), true);
153+
TFile filec{filename.c_str(), "RECREATE"};
154+
ASSERT_EQ(filec.TestBit(TFile::k630forwardCompatibility), true);
155155
filec.Close();
156-
TFile filer{filename.c_str(),"READ"};
157-
ASSERT_EQ(filer.TestBit(TFile::k630forwardCompatibility), true);
156+
TFile filer{filename.c_str(), "READ"};
157+
ASSERT_EQ(filer.TestBit(TFile::k630forwardCompatibility), true);
158158
filer.Close();
159-
TFile fileu{filename.c_str(),"UPDATE"};
160-
ASSERT_EQ(fileu.TestBit(TFile::k630forwardCompatibility), true);
159+
TFile fileu{filename.c_str(), "UPDATE"};
160+
ASSERT_EQ(fileu.TestBit(TFile::k630forwardCompatibility), true);
161161
fileu.Close();
162162
gSystem->Unlink(filename.c_str());
163163
}
@@ -203,3 +203,51 @@ TEST(TFile, MakeSubDirectory)
203203
EXPECT_EQ(std::string(gDirectory->GetPath()), "dirTest17824.root:/a/b/c");
204204
EXPECT_EQ(std::string(gDirectory->GetName()), "c");
205205
}
206+
207+
TEST(TFile, WalkTKeys)
208+
{
209+
struct FileRaii {
210+
std::string fFilename;
211+
FileRaii(std::string_view fname) : fFilename(fname) {}
212+
~FileRaii() { gSystem->Unlink(fFilename.c_str()); }
213+
} fileGuard("tfile_walk_tkeys.root");
214+
215+
TFile outFile(fileGuard.fFilename.c_str(), "RECREATE");
216+
217+
std::string foo = "foo";
218+
outFile.WriteObject(&foo, "foo");
219+
220+
// Write an object with an extremely long name (> 128 chars but < 256)
221+
static const char kLongKey[] = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
222+
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
223+
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
224+
static_assert(std::size(kLongKey) > 128);
225+
static_assert(std::size(kLongKey) < 256);
226+
outFile.WriteObject(&foo, kLongKey);
227+
228+
// Write an object with an even longer name (> 256 chars)
229+
static const char kLongerKey[] = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
230+
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
231+
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
232+
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
233+
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
234+
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
235+
static_assert(std::size(kLongerKey) > 256);
236+
outFile.WriteObject(&foo, kLongerKey);
237+
outFile.Close();
238+
239+
TFile inFile(fileGuard.fFilename.c_str(), "READ");
240+
auto keys = inFile.WalkTKeys();
241+
auto it = keys.begin();
242+
EXPECT_EQ(it->fKeyName, "tfile_walk_tkeys.root");
243+
EXPECT_EQ(it->fClassName, "TFile");
244+
++it;
245+
EXPECT_EQ(it->fKeyName, "foo");
246+
EXPECT_EQ(it->fClassName, "string");
247+
++it;
248+
EXPECT_EQ(it->fKeyName, kLongKey);
249+
EXPECT_EQ(it->fClassName, "string");
250+
++it;
251+
EXPECT_EQ(it->fKeyName, kLongerKey);
252+
EXPECT_EQ(it->fClassName, "string");
253+
}

0 commit comments

Comments
 (0)