SedInstance: changing the number of steps after instantiation bypasses its validation (0 steps overruns the results buffers)

VERSION.txt

@@ -1 +1 @@
-1.20261009.0
+1.20261009.1

src/sed/sedinstancetask.cpp

@@ -526,13 +526,25 @@ double SedInstanceTask::Impl::run()
     auto startTime {std::chrono::high_resolution_clock::now()};
 
     // Reset our progress counters.
+    // Note: we retrieve our number of steps only once since it is used in several places below.
 
     const auto *sedUniformTimeCoursePimpl {mDifferentialModel ? mSedUniformTimeCourse->pimpl() : nullptr};
-    const auto totalSteps {mDifferentialModel ? static_cast<size_t>(sedUniformTimeCoursePimpl->mNumberOfSteps) : 1};
+    const auto numberOfSteps {mDifferentialModel ? sedUniformTimeCoursePimpl->mNumberOfSteps : 1};
+    const auto totalSteps {(numberOfSteps > 0) ? static_cast<size_t>(numberOfSteps) : 0};
 
     mCompletedSteps.store(0, std::memory_order_relaxed);
     mTotalSteps.store(totalSteps, std::memory_order_relaxed);
 
+    // Make sure that our number of steps is valid.
+    // Note: our number of steps was validated when we were instantiated (see SedSimulation::Impl::isValid()), but it
+    //       may have been changed since then.
+
+    if (numberOfSteps <= 0) {
+        addIssue(Issue::Type::ERROR, sedUniformTimeCoursePimpl->invalidNumberOfStepsError(mModel, numberOfSteps), "Simulation");
+
+        return 0.0;
+    }
+
     // (Re)initialise our model.
     // Note: reinitialise our model because we initialised it when we created the instance task.
 
@@ -544,7 +556,7 @@ double SedInstanceTask::Impl::run()
         // Run our simulation from the initial time to the output start time, without tracking our results, but only if
         // the output start time is after the initial time.
 
-        const auto voiInterval {(sedUniformTimeCoursePimpl->mOutputEndTime - sedUniformTimeCoursePimpl->mOutputStartTime) / sedUniformTimeCoursePimpl->mNumberOfSteps};
+        const auto voiInterval {(sedUniformTimeCoursePimpl->mOutputEndTime - sedUniformTimeCoursePimpl->mOutputStartTime) / numberOfSteps};
 
         if (!fuzzyCompare(sedUniformTimeCoursePimpl->mInitialTime, sedUniformTimeCoursePimpl->mOutputStartTime)) {
             run(sedUniformTimeCoursePimpl->mInitialTime, sedUniformTimeCoursePimpl->mOutputStartTime, voiInterval, false);
@@ -555,17 +567,28 @@ double SedInstanceTask::Impl::run()
         }
 
         // Initialise our results structure.
+        // Note: we release our previous results first (to limit our peak memory usage) and only then allocate our new
+        //       results, which we do into a local structure that we move into ours once all of its arrays have been
+        //       allocated. This means that should an allocation fail (e.g., std::bad_alloc), our results structure
+        //       would be left empty rather than with an inconsistent results size and arrays of mismatched sizes (which
+        //       would result in out-of-bounds accesses when retrieving our results).
 
         const auto resultsSize {totalSteps + 1};
 
-        mResults.resultsSize = resultsSize;
+        mResults = {};
+
+        SedInstanceTaskResults results;
+
+        results.resultsSize = resultsSize;
+
+        results.voi.resize(resultsSize);
+        results.states.resize(mStateCount * resultsSize);
+        results.rates.resize(mStateCount * resultsSize);
+        results.constants.resize(mConstantCount * resultsSize);
+        results.computedConstants.resize(mComputedConstantCount * resultsSize);
+        results.algebraicVariables.resize(mAlgebraicVariableCount * resultsSize);
 
-        mResults.voi.resize(resultsSize);
-        mResults.states.resize(mStateCount * resultsSize);
-        mResults.rates.resize(mStateCount * resultsSize);
-        mResults.constants.resize(mConstantCount * resultsSize);
-        mResults.computedConstants.resize(mComputedConstantCount * resultsSize);
-        mResults.algebraicVariables.resize(mAlgebraicVariableCount * resultsSize);
+        mResults = std::move(results);
 
         // Run our simulation from the output start time to the output end time, tracking our results.
 

src/sed/sedsimulation.cpp

@@ -33,6 +33,25 @@ SedSimulation::Impl::Impl(const SedDocumentPtr &pDocument)
 {
 }
 
+std::string SedSimulation::Impl::invalidNumberOfStepsError(const SedModelPtr &pModel, int pNumberOfSteps) const
+{
+    const auto &modelId = pModel->pimpl()->mId;
+    const auto numberOfSteps {toString(pNumberOfSteps)};
+    std::string res;
+
+    res.reserve(mId.size() + modelId.size() + numberOfSteps.size() + 112); // NOLINT
+
+    res += "Simulation '";
+    res += mId;
+    res += "' is to be used with model '";
+    res += modelId;
+    res += "' which requires a strictly positive number of steps but ";
+    res += numberOfSteps;
+    res += " is provided.";
+
+    return res;
+}
+
 bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse)
 {
     auto modelType {pModel->pimpl()->mFile->pimpl()->mCellmlFile->type()};
@@ -77,20 +96,7 @@ bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTim
 
             addError(error);
         } else if (pUniformTimeCourse->numberOfSteps() <= 0) {
-            const auto numberOfSteps {toString(pUniformTimeCourse->numberOfSteps())};
-            std::string error;
-
-            error.reserve(mId.size() + modelId.size() + numberOfSteps.size() + 112); // NOLINT
-
-            error += "Simulation '";
-            error += mId;
-            error += "' is to be used with model '";
-            error += modelId;
-            error += "' which requires a strictly positive number of steps but ";
-            error += numberOfSteps;
-            error += " is provided.";
-
-            addError(error);
+            addError(invalidNumberOfStepsError(pModel, pUniformTimeCourse->numberOfSteps()));
         }
     }
 

src/sed/sedsimulation_p.h

@@ -30,6 +30,8 @@ class SedSimulation::Impl: public SedBase::Impl
 
     explicit Impl(const SedDocumentPtr &pDocument);
 
+    std::string invalidNumberOfStepsError(const SedModelPtr &pModel, int pNumberOfSteps) const;
+
     bool isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse);
 
     const SolverOdePtr &odeSolver() const;

tests/api/sed/instancetests.cpp

@@ -675,6 +675,33 @@ TEST(InstanceSedTest, odeModelWithInvalidNumberOfSteps)
     EXPECT_FALSE(instance->hasIssues());
     EXPECT_GT(instance->run(), 0.0);
     EXPECT_FALSE(instance->hasIssues());
+
+    // Make sure that an invalid number of steps is reported when running an instance even if it was valid when the
+    // instance was created.
+
+    const auto &instanceTask {instance->tasks()[0]};
+
+    simulation->setNumberOfSteps(0);
+
+    EXPECT_EQ(instance->run(), 0.0);
+    EXPECT_EQ_ISSUES(instance, ZERO_STEPS_EXPECTED_ISSUES);
+    EXPECT_EQ(instanceTask->voi().size(), 1001);
+
+    simulation->setNumberOfSteps(-100); // NOLINT
+
+    EXPECT_TRUE(instance->startRun());
+    EXPECT_EQ(instance->waitForRun(), 0.0);
+    EXPECT_EQ(instance->status(), libOpenCOR::SedInstance::Status::IDLE);
+    EXPECT_EQ_ISSUES(instance, NEGATIVE_STEPS_EXPECTED_ISSUES);
+    EXPECT_EQ(instanceTask->voi().size(), 1001);
+
+    // Make sure that the instance can be run again once the number of steps has been fixed.
+
+    simulation->setNumberOfSteps(500); // NOLINT
+
+    EXPECT_GT(instance->run(), 0.0);
+    EXPECT_FALSE(instance->hasIssues());
+    EXPECT_EQ(instanceTask->voi().size(), 501);
 }
 
 TEST(InstanceSedTest, nlaModel)

tests/bindings/javascript/sed.instance.test.js

@@ -705,12 +705,14 @@ test.describe('Sed instance tests', () => {
 
     let instance = document.instantiate();
 
-    assertIssues(loc, instance, [
+    const zeroStepsExpectedIssues = [
       [
         loc.Issue.Type.ERROR,
         "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but 0 is provided."
       ]
-    ]);
+    ];
+
+    assertIssues(loc, instance, zeroStepsExpectedIssues);
 
     simulation.numberOfSteps = -100;
 
@@ -741,6 +743,33 @@ test.describe('Sed instance tests', () => {
     assert.strictEqual(instance.hasIssues, false);
     assert.ok(instance.run() > 0.0);
     assert.strictEqual(instance.hasIssues, false);
+
+    // Make sure that an invalid number of steps is reported when running an instance even if it was valid when the
+    // instance was created.
+
+    const instanceTask = instance.tasks[0];
+
+    simulation.numberOfSteps = 0;
+
+    assert.strictEqual(instance.run(), 0.0);
+    assertIssues(loc, instance, zeroStepsExpectedIssues);
+    assert.strictEqual(instanceTask.voi.length, 1001);
+
+    simulation.numberOfSteps = -100;
+
+    assert.strictEqual(instance.startRun(), true);
+    assert.strictEqual(instance.waitForRun(), 0.0);
+    assert.strictEqual(instance.status, loc.SedInstance.Status.IDLE);
+    assertIssues(loc, instance, negativeStepsExpectedIssues);
+    assert.strictEqual(instanceTask.voi.length, 1001);
+
+    // Make sure that the instance can be run again once the number of steps has been fixed.
+
+    simulation.numberOfSteps = 500;
+
+    assert.ok(instance.run() > 0.0);
+    assert.strictEqual(instance.hasIssues, false);
+    assert.strictEqual(instanceTask.voi.length, 501);
   });
 
   test('NLA model', () => {

tests/bindings/python/test_sed_instance.py

@@ -667,6 +667,33 @@ def test_ode_model_with_invalid_number_of_steps():
     assert instance.run() > 0.0
     assert not instance.has_issues
 
+    # Make sure that an invalid number of steps is reported when running an instance even if it was valid when the
+    # instance was created.
+
+    instance_task = instance.tasks[0]
+
+    simulation.number_of_steps = 0
+
+    assert instance.run() == 0.0
+    assert_issues(instance, zero_steps_expected_issues)
+    assert len(instance_task.voi) == 1001
+
+    simulation.number_of_steps = -100
+
+    assert instance.start_run() is True
+    assert instance.wait_for_run() == 0.0
+    assert instance.status == loc.SedInstance.Status.Idle
+    assert_issues(instance, negative_steps_expected_issues)
+    assert len(instance_task.voi) == 1001
+
+    # Make sure that the instance can be run again once the number of steps has been fixed.
+
+    simulation.number_of_steps = 500
+
+    assert instance.run() > 0.0
+    assert not instance.has_issues
+    assert len(instance_task.voi) == 501
+
 
 def test_nla_model():
     expected_issues = [