From e102537090c85ce222c35985d934306b600520f4 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Tue, 11 Jun 2024 22:39:47 +0200 Subject: [PATCH] [tree] Avoid heap-use-after-free in BulkApiVarLength test ``` 479: ==2573107==ERROR: AddressSanitizer: heap-use-after-free on address 0x617000051b48 at pc 0x7f0fcf4e089e bp 0x7fff6e7e1fe0 sp 0x7fff6e7e1fd8 479: READ of size 8 at 0x617000051b48 thread T0 479: #0 0x7f0fcf4e089d in TTree::GetNotify() const /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/inc/TTree.h:503 479: #1 0x7f0fcf4e089d in void TNotifyLinkBase::RemoveLink(TTree&) /home/vpadulan/Programs/rootproject/rootsrc/core/base/inc/TNotifyLin k.h:104 479: #2 0x7f0fcf4e089d in TTreeReader::~TTreeReader() /home/vpadulan/Programs/rootproject/rootsrc/tree/treeplayer/src/TTreeReader.cxx:252 479: #3 0x4321ca in BulkApiVariableTest_stdRead_Test::TestBody() /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/test/BulkApiVarLength.c xx:135 479: #4 0x470c8c in void testing::internal::HandleExceptionsInMethodIfSupported(testing::Test*, void (testing::Test::*)() , char const*) (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApiVarLength+0x470 c8c) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) 479: #5 0x45a6d3 in testing::Test::Run() [clone .part.0] (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing- asan/tree/tree/test/testBulkApiVarLength+0x45a6d3) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) 479: #6 0x45aa49 in testing::TestInfo::Run() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tr ee/test/testBulkApiVarLength+0x45aa49) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) 479: #7 0x45abf0 in testing::TestSuite::Run() [clone .part.0] (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-tes ting-asan/tree/tree/test/testBulkApiVarLength+0x45abf0) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) 479: #8 0x46769e in testing::internal::UnitTestImpl::RunAllTests() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-fre e-testing-asan/tree/tree/test/testBulkApiVarLength+0x46769e) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) 479: #9 0x45b04c in testing::UnitTest::Run() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tr ee/test/testBulkApiVarLength+0x45b04c) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) 479: #10 0x424606 in main (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApi VarLength+0x424606) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) 479: 0x617000051b48 is located 328 bytes inside of 712-byte region [0x617000051a00,0x617000051cc8) 479: freed by thread T0 here: 479: #0 0x7f0fcf8da878 in operator delete(void*) (/lib64/libasan.so.8+0xda878) (BuildId: 2e1c50524ff1a2e7e73c4565b46f3f51892353ea) 479: #1 0x7f0fcb9b4f25 in TCollection::GarbageCollect(TObject*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/TCollection.cxx:736 479: #2 0x7f0fcb9e8a27 in TList::Delete(char const*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/TList.cxx:535 479: #3 0x7f0fcb9c53d7 in THashList::Delete(char const*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/THashList.cxx:215 479: #4 0x7f0fcc2d285d in TDirectoryFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TDirectoryFile.cxx:585 479: #5 0x7f0fcc2d285d in TDirectoryFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TDirectoryFile.cxx:561 479: #6 0x7f0fcc3468e4 in TFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:989 479: #7 0x7f0fcc3481fd in TFile::~TFile() /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:566 479: #8 0x7f0fcc348fd0 in TFile::~TFile() /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:603 479: #9 0x432ebf in BulkApiVariableTest_stdRead_Test::TestBody() /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/test/BulkApiVarLength.c xx:130 479: #10 0x470c8c in void testing::internal::HandleExceptionsInMethodIfSupported(testing::Test*, void (testing::Test::*)( ), char const*) (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApiVarLength+0x47 0c8c) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56) ``` --- tree/tree/test/BulkApiVarLength.cxx | 40 +++++++++++++---------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/tree/tree/test/BulkApiVarLength.cxx b/tree/tree/test/BulkApiVarLength.cxx index 025925c3cf05ac..d8f3f2ae2fac7e 100644 --- a/tree/tree/test/BulkApiVarLength.cxx +++ b/tree/tree/test/BulkApiVarLength.cxx @@ -24,15 +24,15 @@ class BulkApiVariableTest : public ::testing::Test { protected: void SetUp() override { - auto hfile = new TFile(fFileName.c_str(), "RECREATE", "TTree float micro benchmark ROOT file"); - hfile->SetCompressionLevel(0); // No compression at all. + TFile hfile{fFileName.c_str(), "RECREATE", "TTree float micro benchmark ROOT file"}; + hfile.SetCompressionLevel(0); // No compression at all. - auto tree = new TTree("T", "A ROOT tree of variable-length primitive branches."); - tree->SetBit(TTree::kOnlyFlushAtCluster); - tree->SetAutoFlush(fClusterSize); + TTree tree{"T", "A ROOT tree of variable-length primitive branches."}; + tree.SetBit(TTree::kOnlyFlushAtCluster); + tree.SetAutoFlush(fClusterSize); ROOT::TIOFeatures features; features.Set(ROOT::Experimental::EIOFeatures::kGenerateOffsetMap); - tree->SetIOFeatures(features); + tree.SetIOFeatures(features); float f_counter = 0; float f[10]; @@ -40,10 +40,10 @@ class BulkApiVariableTest : public ::testing::Test { int i[10]; int myLen = 0; - tree->Branch("myLen", &myLen, "myLen/I", 32000); - auto branch2 = tree->Branch("f", &f, "f[myLen]/F", 32000); - auto branch3 = tree->Branch("d", &d, "d[myLen]/D", 32000); - auto branch4 = tree->Branch("i", &i, "i[myLen]/I", 32000); + tree.Branch("myLen", &myLen, "myLen/I", 32000); + auto branch2 = tree.Branch("f", &f, "f[myLen]/F", 32000); + auto branch3 = tree.Branch("d", &d, "d[myLen]/D", 32000); + auto branch4 = tree.Branch("i", &i, "i[myLen]/I", 32000); branch2->SetAutoDelete(false); branch3->SetAutoDelete(false); branch4->SetAutoDelete(false); @@ -56,14 +56,11 @@ class BulkApiVariableTest : public ::testing::Test { } myLen = ev % 10; - tree->Fill(); - } - hfile = tree->GetCurrentFile(); - hfile->Write(); - tree->Print(); + tree.Fill(); + } + hfile.Write(); + tree.Print(); printf("Successful write of all events.\n"); - - delete hfile; } }; @@ -72,13 +69,13 @@ constexpr Long64_t BulkApiVariableTest::fEventCount; TEST_F(BulkApiVariableTest, stdRead) { - auto hfile = TFile::Open(fFileName.c_str()); + std::unique_ptr hfile{TFile::Open(fFileName.c_str())}; printf("Starting read of file %s.\n", fFileName.c_str()); TStopwatch sw; printf("Using standard read APIs.\n"); - TTreeReader myReader("T", hfile); + TTreeReader myReader("T", hfile.get()); TTreeReaderArray myF(myReader, "f"); TTreeReaderArray myD(myReader, "d"); TTreeReaderValue myI(myReader, "myLen"); @@ -126,8 +123,7 @@ TEST_F(BulkApiVariableTest, stdRead) } ev++; } - ASSERT_EQ(ev, events+1); - delete hfile; + ASSERT_EQ(ev, events + 1); sw.Stop(); printf("TTreeReader: Successful read of all events.\n"); @@ -136,7 +132,7 @@ TEST_F(BulkApiVariableTest, stdRead) TEST_F(BulkApiVariableTest, serializedRead) { - auto hfile = TFile::Open(fFileName.c_str()); + std::unique_ptr hfile{TFile::Open(fFileName.c_str())}; printf("Starting read of file %s.\n", fFileName.c_str()); TStopwatch sw;