Destroy Simulation and Universe when destroying CelestiaCore

- Fix use-after-free when destroying Timelines
- Use single ownership of TimelinePhase
- celx phase routines use non-owning pointers

Author: ajtribick

src/celengine/axisarrow.cpp

@@ -220,8 +220,8 @@ VelocityVectorArrow::VelocityVectorArrow(const Body& _body) :
 Eigen::Vector3d
 VelocityVectorArrow::getDirection(double tdb) const
 {
-    const TimelinePhase* phase = body.getTimeline()->findPhase(tdb).get();
-    return phase->orbitFrame()->getOrientation(tdb).conjugate() * phase->orbit()->velocityAtTime(tdb);
+    const TimelinePhase& phase = body.getTimeline()->findPhase(tdb);
+    return phase.orbitFrame()->getOrientation(tdb).conjugate() * phase.orbit()->velocityAtTime(tdb);
 }
 
 std::string_view
@@ -278,8 +278,8 @@ SpinVectorArrow::SpinVectorArrow(const Body& _body) :
 Eigen::Vector3d
 SpinVectorArrow::getDirection(double tdb) const
 {
-    const TimelinePhase* phase = body.getTimeline()->findPhase(tdb).get();
-    return phase->bodyFrame()->getOrientation(tdb).conjugate() * phase->rotationModel()->angularVelocityAtTime(tdb);
+    const TimelinePhase& phase = body.getTimeline()->findPhase(tdb);
+    return phase.bodyFrame()->getOrientation(tdb).conjugate() * phase.rotationModel()->angularVelocityAtTime(tdb);
 }
 
 std::string_view

src/celengine/body.cpp

@@ -255,25 +255,25 @@ Body::markUpdated()
 const std::shared_ptr<const ReferenceFrame>&
 Body::getOrbitFrame(double tdb) const
 {
-    return timeline->findPhase(tdb)->orbitFrame();
+    return timeline->findPhase(tdb).orbitFrame();
 }
 
 const celestia::ephem::Orbit*
 Body::getOrbit(double tdb) const
 {
-    return timeline->findPhase(tdb)->orbit().get();
+    return timeline->findPhase(tdb).orbit().get();
 }
 
 const std::shared_ptr<const ReferenceFrame>&
 Body::getBodyFrame(double tdb) const
 {
-    return timeline->findPhase(tdb)->bodyFrame();
+    return timeline->findPhase(tdb).bodyFrame();
 }
 
 const celestia::ephem::RotationModel*
 Body::getRotationModel(double tdb) const
 {
-    return timeline->findPhase(tdb)->rotationModel().get();
+    return timeline->findPhase(tdb).rotationModel().get();
 }
 
 /*! Get the radius of a sphere large enough to contain the primary
@@ -581,16 +581,16 @@ Body::getPosition(double tdb) const
 {
     Eigen::Vector3d position = Eigen::Vector3d::Zero();
 
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
-    Eigen::Vector3d p = phase->orbit()->positionAtTime(tdb);
-    const ReferenceFrame* frame = phase->orbitFrame().get();
+    const TimelinePhase& phase = timeline->findPhase(tdb);
+    Eigen::Vector3d p = phase.orbit()->positionAtTime(tdb);
+    const ReferenceFrame* frame = phase.orbitFrame().get();
 
     while (frame->getCenter().getType() == SelectionType::Body)
     {
-        phase = frame->getCenter().body()->timeline->findPhase(tdb).get();
+        const TimelinePhase& centerPhase = frame->getCenter().body()->timeline->findPhase(tdb);
         position += frame->getOrientation(tdb).conjugate() * p;
-        p = phase->orbit()->positionAtTime(tdb);
-        frame = phase->orbitFrame().get();
+        p = centerPhase.orbit()->positionAtTime(tdb);
+        frame = centerPhase.orbitFrame().get();
     }
 
     position += frame->getOrientation(tdb).conjugate() * p;
@@ -606,20 +606,20 @@ Body::getPosition(double tdb) const
 Eigen::Quaterniond
 Body::getOrientation(double tdb) const
 {
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
-    return phase->rotationModel()->orientationAtTime(tdb) * phase->bodyFrame()->getOrientation(tdb);
+    const TimelinePhase& phase = timeline->findPhase(tdb);
+    return phase.rotationModel()->orientationAtTime(tdb) * phase.bodyFrame()->getOrientation(tdb);
 }
 
 /*! Get the velocity of the body in the universal frame.
  */
 Eigen::Vector3d
 Body::getVelocity(double tdb) const
 {
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
+    const TimelinePhase& phase = timeline->findPhase(tdb);
 
-    const ReferenceFrame* orbitFrame = phase->orbitFrame().get();
+    const ReferenceFrame* orbitFrame = phase.orbitFrame().get();
 
-    Eigen::Vector3d v = phase->orbit()->velocityAtTime(tdb);
+    Eigen::Vector3d v = phase.orbit()->velocityAtTime(tdb);
     v = orbitFrame->getOrientation(tdb).conjugate() * v + orbitFrame->getCenter().getVelocity(tdb);
 
     if (!orbitFrame->isInertial())
@@ -636,11 +636,11 @@ Body::getVelocity(double tdb) const
 Eigen::Vector3d
 Body::getAngularVelocity(double tdb) const
 {
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
+    const TimelinePhase& phase = timeline->findPhase(tdb);
 
-    Eigen::Vector3d v = phase->rotationModel()->angularVelocityAtTime(tdb);
+    Eigen::Vector3d v = phase.rotationModel()->angularVelocityAtTime(tdb);
 
-    const ReferenceFrame* bodyFrame = phase->bodyFrame().get();
+    const ReferenceFrame* bodyFrame = phase.bodyFrame().get();
     v = bodyFrame->getOrientation(tdb).conjugate() * v;
     if (!bodyFrame->isInertial())
     {
@@ -659,8 +659,8 @@ Body::getAngularVelocity(double tdb) const
 Eigen::Matrix4d
 Body::getLocalToAstrocentric(double tdb) const
 {
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
-    Eigen::Vector3d p = phase->orbitFrame()->convertToAstrocentric(phase->orbit()->positionAtTime(tdb), tdb);
+    const TimelinePhase& phase = timeline->findPhase(tdb);
+    Eigen::Vector3d p = phase.orbitFrame()->convertToAstrocentric(phase.orbit()->positionAtTime(tdb), tdb);
     return Eigen::Transform<double, 3, Eigen::Affine>(Eigen::Translation3d(p)).matrix();
 }
 
@@ -670,17 +670,17 @@ Eigen::Vector3d
 Body::getAstrocentricPosition(double tdb) const
 {
     // TODO: Switch the iterative method used in getPosition
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
-    return phase->orbitFrame()->convertToAstrocentric(phase->orbit()->positionAtTime(tdb), tdb);
+    const TimelinePhase& phase = timeline->findPhase(tdb);
+    return phase.orbitFrame()->convertToAstrocentric(phase.orbit()->positionAtTime(tdb), tdb);
 }
 
 /*! Get a rotation that converts from the ecliptic frame to the body frame.
  */
 Eigen::Quaterniond
 Body::getEclipticToFrame(double tdb) const
 {
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
-    return phase->bodyFrame()->getOrientation(tdb);
+    const TimelinePhase& phase = timeline->findPhase(tdb);
+    return phase.bodyFrame()->getOrientation(tdb);
 }
 
 /*! Get a rotation that converts from the ecliptic frame to the body's
@@ -689,8 +689,8 @@ Body::getEclipticToFrame(double tdb) const
 Eigen::Quaterniond
 Body::getEclipticToEquatorial(double tdb) const
 {
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
-    return phase->rotationModel()->equatorOrientationAtTime(tdb) * phase->bodyFrame()->getOrientation(tdb);
+    const TimelinePhase& phase = timeline->findPhase(tdb);
+    return phase.rotationModel()->equatorOrientationAtTime(tdb) * phase.bodyFrame()->getOrientation(tdb);
 }
 
 // The body-fixed coordinate system has an origin at the center of the
@@ -699,8 +699,8 @@ Body::getEclipticToEquatorial(double tdb) const
 Eigen::Quaterniond
 Body::getEquatorialToBodyFixed(double tdb) const
 {
-    const TimelinePhase* phase = timeline->findPhase(tdb).get();
-    return phase->rotationModel()->spin(tdb);
+    const TimelinePhase& phase = timeline->findPhase(tdb);
+    return phase.rotationModel()->spin(tdb);
 }
 
 /*! Get a transformation to convert from the object's body fixed frame

src/celengine/frametree.cpp

@@ -13,6 +13,7 @@
 #include "frametree.h"
 
 #include <algorithm>
+#include <cassert>
 
 #include <celephem/orbit.h>
 #include "frame.h"
@@ -57,7 +58,11 @@ FrameTree::FrameTree(Body* body) :
 {
 }
 
-FrameTree::~FrameTree() = default;
+FrameTree::~FrameTree()
+{
+    for (const auto& phase : children)
+        phase->m_owner = nullptr;
+}
 
 /*! Return the default reference frame for the object a frame tree is associated
  *  with.
@@ -138,8 +143,10 @@ FrameTree::recomputeBoundingSphere()
 /*! Add a new phase to this tree.
  */
 void
-FrameTree::addChild(const std::shared_ptr<const TimelinePhase>& phase)
+FrameTree::addChild(TimelinePhase* phase)
 {
+    assert(phase->m_owner == nullptr);
+    phase->m_owner = this;
     children.push_back(phase);
     markChanged();
 }
@@ -148,12 +155,14 @@ FrameTree::addChild(const std::shared_ptr<const TimelinePhase>& phase)
  *  phase doesn't exist in the tree.
  */
 void
-FrameTree::removeChild(const std::shared_ptr<const TimelinePhase>& phase)
+FrameTree::removeChild(TimelinePhase* phase)
 {
     auto iter = std::find(children.begin(), children.end(), phase);
     if (iter != children.end())
     {
         children.erase(iter);
+        assert(phase->m_owner == this);
+        phase->m_owner = nullptr;
         markChanged();
     }
 }
@@ -162,7 +171,8 @@ FrameTree::removeChild(const std::shared_ptr<const TimelinePhase>& phase)
 const TimelinePhase*
 FrameTree::getChild(unsigned int n) const
 {
-    return children[n].get();
+    assert(n < children.size());
+    return children[n];
 }
 
 /*! Get the number of immediate children of this tree. */

src/celengine/frametree.h

@@ -28,6 +28,11 @@ class FrameTree //NOSONAR
     explicit FrameTree(Body*);
     ~FrameTree();
 
+    FrameTree(const FrameTree&) = delete;
+    FrameTree& operator=(const FrameTree&) = delete;
+    FrameTree(FrameTree&&) = delete;
+    FrameTree& operator=(FrameTree&&) = delete;
+
     /*! Return the star that this tree is associated with; it will be
      *  nullptr for frame trees associated with solar system bodies.
      */
@@ -38,8 +43,6 @@ class FrameTree //NOSONAR
 
     const std::shared_ptr<const ReferenceFrame>& getDefaultReferenceFrame() const;
 
-    void addChild(const std::shared_ptr<const TimelinePhase>& phase);
-    void removeChild(const std::shared_ptr<const TimelinePhase>& phase);
     const TimelinePhase* getChild(unsigned int n) const;
     unsigned int childCount() const;
 
@@ -89,9 +92,12 @@ class FrameTree //NOSONAR
     }
 
 private:
+    void addChild(TimelinePhase* phase);
+    void removeChild(TimelinePhase* phase);
+
     Star* starParent{ nullptr };
     Body* bodyParent{ nullptr };
-    std::vector<std::shared_ptr<const TimelinePhase>> children;
+    std::vector<TimelinePhase*> children;
 
     double m_boundingSphereRadius{ 0.0 };
     double m_maxChildRadius{ 0.0 };
@@ -100,4 +106,6 @@ class FrameTree //NOSONAR
     BodyClassification m_childClassMask{ BodyClassification::EmptyMask };
 
     std::shared_ptr<const ReferenceFrame> defaultFrame;
+
+    friend class TimelinePhase;
 };

src/celengine/render.cpp

@@ -3581,11 +3581,11 @@ void Renderer::buildLabelLists(const math::InfiniteFrustum& viewFrustum,
         if (body->getName().empty())
             continue;
 
-        const TimelinePhase* phase = body->getTimeline()->findPhase(now).get();
-        const Body* primary = phase->orbitFrame()->getCenter().body();
+        const TimelinePhase& phase = body->getTimeline()->findPhase(now);
+        const Body* primary = phase.orbitFrame()->getCenter().body();
         if (primary != nullptr && util::is_set(primary->getClassification(), BodyClassification::Invisible))
         {
-            const Body* parent = phase->orbitFrame()->getCenter().body();
+            const Body* parent = phase.orbitFrame()->getCenter().body();
             if (parent != nullptr)
                 primary = parent;
         }
@@ -3617,8 +3617,8 @@ void Renderer::buildLabelLists(const math::InfiniteFrustum& viewFrustum,
             // position.
             if (primary != lastPrimary)
             {
-                Vector3d p = phase->orbitFrame()->getOrientation(now).conjugate() *
-                             phase->orbit()->positionAtTime(now);
+                Vector3d p = phase.orbitFrame()->getOrientation(now).conjugate() *
+                             phase.orbit()->positionAtTime(now);
                 Vector3d v = ri.position.cast<double>() - p;
 
                 primarySphere = math::Sphered(v, primary->getRadius());

src/celengine/simulation.cpp

@@ -14,15 +14,17 @@
 
 #include <algorithm>
 #include <cstddef>
+#include <utility>
 
 #include <celutil/strnatcmp.h>
 #include "body.h"
 #include "location.h"
 #include "render.h"
 
 
-Simulation::Simulation(Universe* universe, const std::shared_ptr<celestia::engine::ObserverSettings>& observerSettings) :
-    universe(universe)
+Simulation::Simulation(std::unique_ptr<Universe>&& universe,
+                       const std::shared_ptr<celestia::engine::ObserverSettings>& observerSettings) :
+    universe(std::move(universe))
 {
     activeObserver = new Observer(observerSettings);
     observers.push_back(activeObserver);
@@ -63,7 +65,7 @@ void Simulation::render(Renderer& renderer, Observer& observer)
 
 Universe* Simulation::getUniverse() const
 {
-    return universe;
+    return universe.get();
 }
 
 

src/celengine/simulation.h

@@ -33,7 +33,8 @@ class Renderer;
 class Simulation
 {
 public:
-    Simulation(Universe*, const std::shared_ptr<celestia::engine::ObserverSettings>&);
+    Simulation(std::unique_ptr<Universe>&&,
+               const std::shared_ptr<celestia::engine::ObserverSettings>&);
     ~Simulation();
 
     double getTime() const; // Julian date
@@ -131,7 +132,7 @@ class Simulation
     double storedTimeScale{ 1.0 };
     bool syncTime{ true };
 
-    Universe* universe;
+    std::unique_ptr<Universe> universe;
 
     mutable std::optional<SolarSystem*> closestSolarSystem{ std::nullopt };
     Selection selection;