Add bounds-checking to ObjectProperty and appropriate tests against r…

CHANGELOG.md

@@ -15,6 +15,7 @@ been added to these forces to provide access to concrete path types (e.g., `updP
 `getGeometryPath`, `updGeometryPath`, etc., or a suitable alternative.    
 - Fixed a minor memory leak when calling `OpenSim::CoordinateCouplerConstraint::setFunction` (#3541)
 - Increase the number of input dimensions supported by `MultivariatePolynomialFunction` to 6 (#3386)
+- Fixed mis-indexing into an `OpenSim::ObjectProperty` now throws an exception rather than segfaulting (#3347)
 - Clarified that `OpenSim::Controller`'s `actuator_list` takes a list of actuator names, rather than paths (#3484)
 - Deleting elements from an `OpenSim::Coordinate` range now throws an exception during `finalizeFromProperties` (previously:
   it would let you do it, and throw later when `Coordinate::getMinRange()` or `Coordinate::getMaxRange()` were called, #3532)

OpenSim/Common/Object.h

@@ -1449,7 +1449,7 @@ ObjectProperty<T>::setValueAsObject(const Object& obj, int index) {
             + " which can't be stored in this " + objectClassName
             + " property " + this->getName());
 
-    objects[index] = newObjT;
+    objects.at(index) = newObjT;
 }
 /** @endcond **/
 

OpenSim/Common/Property.h

@@ -1151,13 +1151,13 @@ class ObjectProperty : public Property<T> {
     const Object& getValueAsObject(int index=-1) const override final {
         if (index < 0 && this->getMinListSize()==1 && this->getMaxListSize()==1)
             index = 0;
-        return *objects[index];
+        return *objects.at(index);
     }
 
     Object& updValueAsObject(int index=-1) override final {
         if (index < 0 && this->getMinListSize()==1 && this->getMaxListSize()==1)
             index = 0;
-        return *objects[index];
+        return *objects.at(index);
     }
 
     static bool isA(const AbstractProperty& prop) 
@@ -1187,17 +1187,17 @@ class ObjectProperty : public Property<T> {
     }
     // Remove value at specific index
     void removeValueAtIndexVirtual(int index) override {
-        objects.erase(&objects[index]);
+        objects.erase(&objects.at(index));
     }
 private:
     // Base class checks the index.
     const T& getValueVirtual(int index) const override final 
-    {   return *objects[index]; }
+    {   return *objects.at(index); }
     T& updValueVirtual(int index) override final 
-    {   return *objects[index]; }
+    {   return *objects.at(index); }
     void setValueVirtual(int index, const T& obj) override final
-    {   objects[index].reset((T*)nullptr);
-        objects[index] = obj; }
+    {   objects.at(index).reset((T*)nullptr);
+        objects.at(index) = obj; }
     int appendValueVirtual(const T& obj) override final
     {   objects.push_back();        // add empty element
         objects.back() = obj;       // insert a copy

OpenSim/Common/Test/testComponentInterface.cpp

@@ -22,6 +22,7 @@
  * -------------------------------------------------------------------------- */
 #include <OpenSim/Auxiliary/auxiliaryTestFunctions.h>
 #include <OpenSim/Common/Component.h>
+#include <OpenSim/Common/Function.h>
 #include <OpenSim/Common/Reporter.h>
 #include <OpenSim/Common/TableSource.h>
 #include <OpenSim/Common/STOFileAdapter.h>
@@ -53,6 +54,28 @@ namespace
         OpenSim_DECLARE_OUTPUT(some_output, double, getDoubleOutput, SimTK::Stage::Model);
         OpenSim_DECLARE_SOCKET(some_socket, OpenSim::Component, "some socket comment");
     };
+
+    class ComponentWithOptionalSimpleProperty final : public OpenSim::Component {
+        OpenSim_DECLARE_CONCRETE_OBJECT(ComponentWithOptionalSimpleProperty, OpenSim::Component);
+    public:
+        OpenSim_DECLARE_OPTIONAL_PROPERTY(num, int, "");
+
+        ComponentWithOptionalSimpleProperty()
+        {
+            constructProperty_num();
+        }
+    };
+
+    class ComponentWithOptionalObjectProperty final : public OpenSim::Component {
+        OpenSim_DECLARE_CONCRETE_OBJECT(ComponentWithOptionalObjectProperty, OpenSim::Component);
+    public:
+        OpenSim_DECLARE_OPTIONAL_PROPERTY(func, OpenSim::Function, "");
+
+        ComponentWithOptionalObjectProperty()
+        {
+            constructProperty_func();
+        }
+    };
 }
 
 using namespace OpenSim;
@@ -2603,6 +2626,43 @@ void testCacheVariableInterface() {
     }
 }
 
+// repro related to #3409/#3411
+//
+// if downstream code tries to read an optional SimpleProperty without
+// checking whether it's populated or not, it should throw an exception
+// rather than segfaulting
+void testIncorrectlyReadingAnOptionalSimplePropertyDoesNotSegfault()
+{
+    ComponentWithOptionalSimpleProperty c;
+
+    ASSERT_EQUAL(c.getProperty_num().size(), 0);
+    try {
+        // shouldn't segfault...
+        ASSERT_EQUAL(c.get_num(), 1337);
+    } catch (const std::exception&) {
+        // ... but should throw an exception
+    }
+}
+
+// repro related to #3347
+//
+// if downstream code (e.g. OpenSim::CoordinateCouplerConstraint in
+// OpenSim <=4.4) tries to read an optional ObjectProperty without checking
+// whether it's populated or not, it should throw an exception rather
+// than segfaulting
+void testIncorrectlyReadingAnOptionalObjectPropertyDoesNotSegfault()
+{
+    ComponentWithOptionalObjectProperty c;
+
+    ASSERT_EQUAL(c.getProperty_func().size(), 0);
+    try {
+        // shouldn't segfault...
+        c.get_func().getName();
+    } catch (const std::exception&) {
+        // ... but should throw an exception
+    }
+}
+
 int main() {
 
     //Register new types for testing deserialization
@@ -2645,6 +2705,9 @@ int main() {
 
         SimTK_SUBTEST(testFormattedDateTime);
         SimTK_SUBTEST(testCacheVariableInterface);
+        SimTK_SUBTEST(testIncorrectlyReadingAnOptionalSimplePropertyDoesNotSegfault);
+        SimTK_SUBTEST(testIncorrectlyReadingAnOptionalObjectPropertyDoesNotSegfault);
+
 
     SimTK_END_TEST();
 }

OpenSim/Simulation/SimbodyEngine/Test/testConstraints.cpp

@@ -934,6 +934,21 @@ void testCoordinateCouplerConstraint()
 
     // Forces were held in storage during simulation, now write to file
     forceReport->printResults("CouplerModelForces");
+
+    // repro for #3347: creating a `CoordinateCouplerConstraint` with no
+    // `coupled_coordinates_function` should throw a runtime exception
+    // rather than segfault
+    {
+        OpenSim::Model m;
+        m.addConstraint(new OpenSim::CoordinateCouplerConstraint{});
+
+        try {
+            // this shouldn't segfault...
+            m.buildSystem();
+        } catch (const std::exception&) {
+            // ... but it is permitted to throw
+        }
+    }
 }
 
 void testRollingOnSurfaceConstraint()