DiscreteTrajectoryBase push_back unsafe

[markusgft]

@jcarius has reported the following:

The method void push_back(const T& data, const SCALAR& time, const bool timeIsAbsolute) is unsafe in DiscreteTrajectoryBase:

if the user specifies timeIsAbsolute=false, access to time_.back() might segfault or cause undefined behavior because time vector could still be empty.

[sentry5588]

In an effort of helping resolving this issue. I noticed that there is Test for DiscreteArray and DiscreteTrajectory. But there is no Test for DiscreteTrajectoryBase. Is this the reason that this bug was not discovered before? Should DiscreteTrajectoryBaseTest.cpp be created?

[markusgft]

Hi, thanks for raising this. In fact, the CT as it is now has generally little test coverage. This is one of the main drawbacks of the library. If you like you can create the test, but it is not critical. Best!

Am 16.03.2021 um 23:01 schrieb sentry5588 @.***>:

 In the effort of helping resolving this issue. I noticed that there is Test for DiscreteArray and DiscreteTrajectory. But there is no Test for DiscreteTrajectoryBase. Is this the reason that this bug was not discovered before? Should DiscreteTrajectoryBaseTest.cpp be created?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[sentry5588]

Thank you @markusgft . I was creating the unit test for DiscreteTrajectoryBase, and realized that ct_core/test/DiscreteTrajectoryTest.cpp is actually testing DiscreteTrajectoryBase. Because

    // trajectory containers with zoh and linear interpolation
    StateTrajectory<2> stateTrajectory_zoh(timeArray, stateArray, InterpolationType::ZOH);
    StateTrajectory<2> stateTrajectory_lin(timeArray, stateArray, InterpolationType::LIN);

The StateTrajectory is a derived class from DiscreteTrajectoryBase. My question, should DiscreteTrajectoryTest.cpp be renamed as DiscreteTrajectoryBaseTest.cpp? Thank you.

[sentry5588]

I'm about to create a test case to produce a situation of this bug, and I realized that tpl::TimeArray<SCALAR> has a length of a minimum of 2. It doesn't seem possible that time could be empty. But I believe from the unit test point of view, the unit shall cover that. Could you please clarify? Thanks.

[markusgft]

Thanks for raising the issue. I believe that someone needs to take a more detailed look and check what is really going on. I currently have no time to reproduce the failures, unfortunately.