Dynamics_RegularTwoTrack: Unchecked parameter access can cause undefined behavior

File sim/src/components/Dynamics_RegularTwoTrack/dynamics_regularTwoTrack.cpp
In DynamicsRegularTwoTrackImplementation::Init() several values are accessed through parameter maps, without proper checking for existence.

Example: radiusTire.SetValue(parameterMapDoubleExternal.find(radiusTire")->second);
Issue: If the key is missing, find delivers std::end(parameterMapDoubleExternal) and ->second accesses some arbitrary value right after the memory or the map.
Solution: The parameters should come from recently introduced VehicleModelParameters

Note: This issue might also affect the Dynamics_TwoTrack module (https://gitlab.eclipse.org/eclipse/simopenpass/simopenpass/-/issues/36)

added Bug label

mentioned in merge request !54 (merged)

This Bug is triggered by the example config set StaticAgentCollision, commit 7d63754d. Module Dynamics_RegularTwoTrack calculates the acceleration of agent 2 as NaN in timestep 0, resulting in AgentUpdater throwing an error.

mentioned in issue #36 (closed)

assigned to @victorhexad

unassigned @mscharfenberg

set weight to 1

changed milestone to %Release v1.0.0

Contribution by Enno.

Clarification of VW process needed.

Any news here? We are maybe encountering this issue in !122 (merged), where the StaticAgentCollision E2E test fails. But we are still investigating.

What is the objective of this test? I don't see a query in the json file for determining success.

When I take the configs, the simulation works and there is a valid collision between two agents.

However, I assume there is a conflict between the parameter import from the VehicleModelsCatalog and the parameters needed from the static SystemConfig, as in the PCM E2E test.

If we include an additional vehicle model in the VehicleModelsCatalog that contains the required parameters, we need to adapt the TwoTrack model, too.

The other option is to include the parameters in the static SystemConfig, am I right?

   <component>
    <id>7</id>
    <library>Dynamics_TwoTrack</library>
    <title>Dynamics_TwoTrack</title>
    <schedule>
     <priority>3</priority>
     <offset>0</offset>
     <cycle>10</cycle>
     <response>0</response>
    </schedule>
    <parameters>
     <parameter>
      <!--parameter's title: Radius of the tires-->
      <id>0</id>
      <type>double</type>
      <unit>m</unit>
      <value>0.3</value>
     </parameter>
     <parameter>
      <!--parameter's title: Tire's max. mu-->
      <id>1</id>
      <type>double</type>
      <unit/>
      <value>1.1</value>
     </parameter>
     <parameter>
      <!--parameter's title: Tire's slide mu-->
      <id>2</id>
      <type>double</type>
      <unit/>
      <value>0.8</value>
     </parameter>
     <parameter>
      <!--parameter's title: Tire's slip at max. mu-->
      <id>3</id>
      <type>double</type>
      <unit/>
      <value>0.1</value>
     </parameter>
     <parameter>
      <!--parameter's title: Engine power-->
      <id>4</id>
      <type>double</type>
      <unit>W</unit>
      <value>100000</value>
     </parameter>
     <parameter>
      <!--parameter's title: Max. brake torque-->
      <id>5</id>
      <type>double</type>
      <unit>Nm</unit>
      <value>-10000</value>
     </parameter>
    </parameters>
    <position>
     <x>1176</x>
     <y>369</y>
    </position>
   </component>

Yes, there is no specialized E2E test for this config (no query in json file). But it is part of the Generic config set, for which determinism tests are enabled. The determinism test for this specific config fails on !122 (merged).

The determinism test runs three invocations (configured in simulationConfig.xml) starting from random seed 1, so the invocations will have random seed 1, 2, and 3. Then there will be three individual runs with a single invocation, each run configured to start at random seed 1, 2, and 3, respectively. The cyclics of runs with matching random seeds are compared afterwards. This comparison shows a difference for random seed 2 of StaticAgentCollision. NOTE: Values for random seeds shortened here for simplicity.

The root cause is that the SystemConfig.xml of Dynamics_**Regular**TwoTrack component is missing the parameters you mentioned above. Another aspect is, that the component tries to access them unconditionally, leading to undefined behavior (dereferencing an end() iterator returned by find()).

Also, #36 (closed) should be considered before putting to much effort into this (duplication between Dynamics_RegularTwoTrack and Dynamics_TwoTrack).

I'll take the config snippet posted above and put it into the SystemConfig.xml part of Dynamics_RegularTwoTrack. If this fixes the execution and the simulation result still looks reasonable, we can happily proceed with !122 (merged). If this is not the case, I'd disable the test for now and re-enable it when handling this issue here.

Thanks a lot for the explaination. The reason for the duplication #36 (closed) was the different dynamics signals, as I remember.

The bigger issue we should tackle anyway wrt static system configs is the different parameter IDs for SystemConfig.xml when the new GUI comes into play.

I like your step 1 (adding the snippet with the parameters), but as step 2, I have another idea (before you disable the E2E test): Just replace Dynamics_RegularTwoTrack with Dynamics_RegularDriving, like @kblenz did in the static system config with the modular driver model: https://gitlab.eclipse.org/eclipse/openpass/opSimulation/-/blob/108-modular-driver/sim/contrib/examples/Configurations/ModularDriver_CalibratedTraffic_2lanes_867KFZ_0k2004LKWpKFZ/SystemConfigRegular.xml

Adding the code snippet seems to work - so !122 (merged) can be merged, right?

https://ci.eclipse.org/openpass/job/openPASS_simulator_build/view/change-requests/job/MR-122/lastSuccessfulBuild/artifact/artifacts/testreport/linux/test_end_to_end.html

I'll have a short look, if all my comments have been addressed.

As our fix for !122 (merged) worked and is also valid, I'd like to close this issue.

@jdobberstein Please open a separate issue, if you additionally want to address your idea of replacing the dynamics component.

Oops... no, actually I don't want to close. Parameters are still unchecked :-)

Let's see if this is still the case in the upcoming proposal by Enno for DynamicsTwoTrack #36 (closed)

assigned to @gwendallucas and unassigned @victorhexad

configs.zip

New configs with Dynamics_RegularDriving instead of TwoTrack & SpawnParameters provoking crashsconfigs_update.zip

Vehicle parameters (e.g. tire radius) are now obtained from the VehicleModelsCatalog with the new modular vehicle dynamics model (!163 (merged))

closed