Segfault when using an `OpenSim::Model` *copy* of a model containing empty property fields

[adamkewley]

This was found in: https://github.com/ComputationalBiomechanicsLab/opensim-creator/issues/621

The following bug reproduction osim:

• https://gist.github.com/adamkewley/0787aee030d6a21f1a5a73f08debf7cd

Will segfault if finalizeFromProperties is called on a copy of an OpenSim::Model constructed from it. I.e. the following code would segfault:

OpenSim::Model m1{"broken_file.osim"};
m1.finalizeFromProperties();
OpenSim::Model m2{m1};
m2.finalizeFromProperties();  // boom

OpenSim Creator makes many copies of models for undo/redo storage and automatic error recovery, so it crashes essentially instantly on loading that buggy file. OpenSim GUI will load the file, but crash after doing something that copies the model, such as running a forward-dynamic simulation. So a practical repro is:

• Save the broken osim somewhere
• Open it in OpenSim GUI
• Run forward-dynamic simulation (green guy)
• The 4.4 GUI crashes

[adamkewley]

It is likely that the bug is a consequence of several confounding factors:

• If a Coordinate is constructed and constructProperties is called, then the underlying OpenSim::SimpleProperty<double> is initialized with a SimTK::Array_<double, int> = {0.0};
• Reading the above (e.g. via get_default_value()) is safe
• Copying the above is safe: it will create a copied property with the same defaulted value
• When an XML document is read, it goes via:
  • OpenSim::Object::updateFromXMLNode
  • OpenSim::AbstractProperty::readFromXMLParentElement
  • OpenSim::SimpleProperty<double>::readFromXMLElement
  • OpenSim::SimpleProperty<double>::readSimplePropertyFromStream
  • SimTK::readUnformatted(std::istream&, SimTK::Array_<double, int>&)
  • Which clears the array and tries to re-fill it with the XML node's content (Array.h)
  • If reading fails, it will not push any element into the array, leaving the supplied (property-owned) array empty
• Reading the above is logically unsafe. However, it will likely behave correctly and with no segfaults because OpenSim::SimpleProperty<dobule>::getValueVirtual does an unchecked index into the values array. clearing that array didn't de-allocate the array's memory--only destruct its contents--so reading the pointed-to (destructed) values will behave as-if the array is valid (it isn't).
• Copying the above is safe. However, SimTK::Array's copy constructor is clever enough to know that it shouldn't allocate memory when copying an empty array, so the copy will be a 0-element array with a nullptr data member.
• Reading the copy is pretty much always unsafe. It will index vs nullptr and segfault (esp. with index == 0, because then you're asking for *nullptr).

This is my working theory, anyway. There is a check for this kind of thing inside OpenSim, but it looks like it's just thrown out of cerr, which doesn't seem to pop up in Visual Studio etc (https://github.com/opensim-org/opensim-core/blob/v4.3.1/OpenSim/Common/Property.h#L885)

[adamkewley]

Yep, the error is silently swallowed, and then undefined memory behavior makes it behave correctly when using the original model (e.g. in OpenSim GUI) but copying the model then creates a condition where the invalid indexing actively throws a segfault. Opening OSC with a console:

[default] [info] enabling backtrace handler
[default] [info] initializing main application window
[default] [info] initializing OpenGL context
[default] [info] OpenGL initialized: info: NVIDIA Corporation, NVIDIA GeForce GTX 1060 6GB/PCIe/SSE2, (3.3.0 NVIDIA 516.94), GLSL 3.30 NVIDIA via Cg compiler
[default] [info] setting locale to US (so that numbers are always in the format '0.x'
[default] [info] removing OpenSim's default log (opensim.log)
[default] [info] attaching OpenSim to this log
[default] [info] registering OpenSim types
[default] [info] registering OpenSim geometry search path to use osc resources
[default] [info] added geometry search path entry: C:/Users/adamk/OneDrive/Desktop/opensim-creator/resources\geometry
[default] [info] showing screen class osc::MainUIScreen
Not enough values for double property default_value; input=''. Expected 1, got 0.
Not enough values for double property default_value; input=''. Expected 1, got 0.
Not enough values for double property default_speed_value; input=''. Expected 1, got 0.
[info] Loaded model OpenSenseModel_trial_1 from file C:\Users\adamk\OneDrive\Desktop\johanna-daumerie\OpenSenseModel_arm_1.osim
[default] [info] Loaded model OpenSenseModel_trial_1 from file C:\Users\adamk\OneDrive\Desktop\johanna-daumerie\OpenSenseModel_arm_1.osim
[default] [error] exception propagated to root of OSC: might be a segfault?
[default] [error] backtrace:
[default] [error]     #0 osc.exe+0x1C632A [0x7FF76445632A]
[default] [error]     #1 osc.exe+0x1C6B92 [0x7FF764456B92]
[default] [error]     #2 KERNELBASE.dll+0x110207 [0x7FFEB60E0207]
[default] [error]     #3 ntdll.dll+0x112569 [0x7FFEB88E2569]
[default] [error]     #4 ntdll.dll+0x113022 [0x7FFEB88E3022]
[default] [error]     #5 ntdll.dll+0xA568E [0x7FFEB887568E]
[default] [error]     #6 ntdll.dll+0x8C876 [0x7FFEB885C876]
[default] [error]     #7 ntdll.dll+0x9CCAE [0x7FFEB886CCAE]
[default] [error]     #8 ntdll.dll+0xA241F [0x7FFEB887241F]
[default] [error]     #9 ntdll.dll+0x514A4 [0x7FFEB88214A4]
[default] [error]     #10 ntdll.dll+0xA0F4E [0x7FFEB8870F4E]
[default] [error]     #11 osimSimulation.dll+0x389856 [0x7FFE07BB9856]
[default] [error]     #12 osimCommon.dll+0x9B380 [0x7FFE7F67B380]
[default] [error]     #13 osimCommon.dll+0x98AC8 [0x7FFE7F678AC8]
[default] [error]     #14 osimCommon.dll+0x9AC75 [0x7FFE7F67AC75]
[default] [error]     #15 osimCommon.dll+0x98AC8 [0x7FFE7F678AC8]
[default] [error] note: backtrace addresses are return addresses, not call addresses (see: https://devblogs.microsoft.com/oldnewthing/20170505-00/?p=96116)
[default] [error] to analyze the backtrace in WinDbg: `ln osc.exe+ADDR`
Segmentation fault

[adamkewley]

I added a bug reproduction test into opensim-creator that minimally exercises this behavior and demonstrates that it is entirely related to copy-constructing the model:

• https://github.com/ComputationalBiomechanicsLab/opensim-creator/blob/1fd39ec3c75b68144c366fa6cbf5c4f0ce02fe38/tests/TestOpenSim.cpp#L131