FairRoot
FairRoot copied to clipboard
CTest errors fail to be caught by the CI workflow
Describe the bug Multiple crashes in ctests are not caught by the CI workflow.
Unused file source in run_reco_timebased.C
https://github.com/FairRootGroup/FairRoot/blob/fa7108b45684c729c2e88b12d7fbf4bbb5a4dc65/examples/advanced/Tutorial3/macro/run_reco_timebased.C#L42-L48
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:
https://github.com/FairRootGroup/FairRoot/blob/fa7108b45684c729c2e88b12d7fbf4bbb5a4dc65/base/steer/FairRootManager.cxx#L136-L141
In FairRun
, the file source is deleted automatically using std::unique_ptr
:
https://github.com/FairRootGroup/FairRoot/blob/fa7108b45684c729c2e88b12d7fbf4bbb5a4dc65/base/steer/FairRun.h#L242-L246
However, deleting from FairRootManager
is sometimes prevented by:
https://github.com/FairRootGroup/FairRoot/blob/fa7108b45684c729c2e88b12d7fbf4bbb5a4dc65/base/steer/FairRun.cxx#L86-L91
But everything falls apart if FairRootManager
is deleted before FairRun
, as has happened in Ctests such as run_digi_timebased.C
:
https://github.com/FairRootGroup/FairRoot/blob/fa7108b45684c729c2e88b12d7fbf4bbb5a4dc65/examples/advanced/Tutorial3/macro/run_digi_timebased.C#L40-L45
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.