FairRoot icon indicating copy to clipboard operation
FairRoot copied to clipboard

CTest errors fail to be caught by the CI workflow

Open YanzhaoW opened this issue 1 year ago • 1 comments

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.

YanzhaoW avatar Jun 26 '23 13:06 YanzhaoW