Describe the bug
Multiple crashes in ctests are not caught by the CI workflow.
Unused file source in run_reco_timebased.C
|
|
|
FairRunAna *fRun = new FairRunAna(); |
|
FairFileSource *fFileSource = new FairFileSource(inFile); |
|
fRun->SetSink(std::make_unique<FairRootFileSink>(outFile)); |
|
fRun->RunWithTimeStamps(); |
|
fRun->SetUseFairLinks(kTRUE); |
|
|
Here even though the file source
fFileSource is initialised, it isn't used by
FairRun. Obviously this should have crashed the program since FairRunAna must have a source to analyze (it does crash the the PR
#1254).
Double deleting of file source in FairRootManager and FairRun
In FairRootManager, the file source is deleted here:
|
delete fSourceChain; |
|
if (fSink) |
|
delete fSink; |
|
if (fSource) |
|
delete fSource; |
|
|
In FairRun, the file source is deleted automatically using std::unique_ptr:
|
FairAlignmentHandler fAlignmentHandler; |
|
|
|
std::unique_ptr<FairSource> fSource{}; //! |
|
|
|
void AlignGeometry() const; |
However, deleting from FairRootManager is sometimes prevented by:
|
LOG(debug) << "Enter Destructor of FairRun"; |
|
|
|
// So that FairRootManager does not try to delete these, because we will do that: |
|
fRootManager->SetSource(nullptr); |
|
fRootManager->SetSink(nullptr); |
|
|
But everything falls apart if FairRootManager is deleted before FairRun, as has happened in Ctests such as run_digi_timebased.C:
|
|
|
// ----- Reconstruction run ------------------------------------------- |
|
FairRunAna* fRun = new FairRunAna(); |
|
FairFileSource* fFileSource = new FairFileSource(inFile); |
|
fRun->SetSource(fFileSource); |
|
|
Here
FairRunAna is initialised with the "evil"
new operator and not deleted at the end of the program. As the result, the destructor of
FairRootManager is executed followed by the destructor of
FairRun. Therefore, we have the double deleting of one file source pointer, which is an UB (some time the program crashes, sometimes not).
Additional context
I have never understood what is the use of this "idiom". It's also used at many places in R3BRoot.
if(a_ptr != nullptr)
delete a_ptr;If a_ptr is a nullptr, deleting a nullptr is totally fine in c++. If a_ptr is not a nullptr but it's been deleted, we have a double deleting anyway. I'm confused which scenario this "idiom" is trying to prevent.
Describe the bug
Multiple crashes in ctests are not caught by the CI workflow.
Unused file source in run_reco_timebased.C
FairRoot/examples/advanced/Tutorial3/macro/run_reco_timebased.C
Lines 42 to 48 in fa7108b
Here even though the file source
fFileSourceis initialised, it isn't used byFairRun. Obviously this should have crashed the program since FairRunAna must have a source to analyze (it does crash the the PR #1254).Double deleting of file source in FairRootManager and FairRun
In
FairRootManager, the file source is deleted here:FairRoot/base/steer/FairRootManager.cxx
Lines 136 to 141 in fa7108b
In
FairRun, the file source is deleted automatically usingstd::unique_ptr:FairRoot/base/steer/FairRun.h
Lines 242 to 246 in fa7108b
However, deleting from
FairRootManageris sometimes prevented by:FairRoot/base/steer/FairRun.cxx
Lines 86 to 91 in fa7108b
But everything falls apart if
FairRootManageris deleted beforeFairRun, as has happened in Ctests such asrun_digi_timebased.C:FairRoot/examples/advanced/Tutorial3/macro/run_digi_timebased.C
Lines 40 to 45 in fa7108b
Here
FairRunAnais initialised with the "evil"newoperator and not deleted at the end of the program. As the result, the destructor ofFairRootManageris executed followed by the destructor ofFairRun. Therefore, we have the double deleting of one file source pointer, which is an UB (some time the program crashes, sometimes not).Additional context
I have never understood what is the use of this "idiom". It's also used at many places in R3BRoot.
If
a_ptris a nullptr, deleting a nullptr is totally fine in c++. Ifa_ptris not a nullptr but it's been deleted, we have a double deleting anyway. I'm confused which scenario this "idiom" is trying to prevent.