From 4a53ec86764d42e8f7b76024c2363f81821b14c5 Mon Sep 17 00:00:00 2001
From: Sandu Liviu Catalin <slc.universe@gmail.com>
Date: Fri, 1 May 2020 01:24:06 +0300
Subject: [PATCH] Prevent issues with routine slot recycling.

Prevent possible memory leak on object creation exceptions.
Other miscellaneous changes.
---
 module/Misc/Routine.cpp |  8 +++++---
 module/Misc/Routine.hpp | 44 +++++++++++++++++++++++++++--------------
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/module/Misc/Routine.cpp b/module/Misc/Routine.cpp
index 2ce450d3..e0463d1c 100644
--- a/module/Misc/Routine.cpp
+++ b/module/Misc/Routine.cpp
@@ -136,12 +136,12 @@ SQInteger Routine::Create(HSQUIRRELVM vm)
     // Is the specified environment a null value?
     if (use_root)
     {
-        // Preserve the stack state
-        const StackGuard sg(vm);
         // Push the root table on the stack
         sq_pushroottable(vm);
         // Attempt to retrieve the table object
         res = sq_getstackobj(vm, -1, &env);
+        // Preserve the stack state
+        sq_poptop(vm);
     }
     // Should we treat it as a valid environment object?
     else if (etype != OT_STRING)
@@ -192,7 +192,9 @@ SQInteger Routine::Create(HSQUIRRELVM vm)
     // Attempt to create a routine instance
     try
     {
-        ClassType< Routine >::PushInstance(vm, new Routine());
+        DeleteGuard< Routine > dg(new Routine());
+        ClassType< Routine >::PushInstance(vm, dg.Get());
+        dg.Release();
     }
     catch (const Sqrat::Exception & e)
     {
diff --git a/module/Misc/Routine.hpp b/module/Misc/Routine.hpp
index 3b9a925f..30e19a7f 100644
--- a/module/Misc/Routine.hpp
+++ b/module/Misc/Routine.hpp
@@ -39,6 +39,7 @@ private:
         bool        mSuspended; // Whether this instance is allowed to receive calls.
         bool        mQuiet; // Whether this instance is allowed to handle errors.
         bool        mEndure; // Whether this instance is allowed to terminate itself on errors.
+        bool        mTerminated; // Whether this instance is allowed to terminate itself on errors.
         Uint8       mArgc; // The number of arguments that the routine must forward.
         Argument    mArgv[14]; // The arguments that the routine must forward.
 
@@ -56,6 +57,7 @@ private:
             , mSuspended(false)
             , mQuiet(GetSilenced())
             , mEndure(false)
+            , mTerminated(false)
             , mArgc(0)
             , mArgv()
         {
@@ -96,19 +98,10 @@ private:
         void Init(HSQOBJECT & env, HSQOBJECT & func, HSQOBJECT & inst, Interval intrv, Iterator itr)
         {
             // Initialize the callback objects
-            if (!sq_isnull(env))
-            {
-                mEnv = LightObj(env);
-            }
-            if (!sq_isnull(func))
-            {
-                mFunc = LightObj(func);
-            }
+            mEnv = LightObj{env};
+            mFunc = LightObj{func};
             // Associate with the routine instance
-            if (!sq_isnull(inst))
-            {
-                mInst = LightObj(inst);
-            }
+            mInst = LightObj{inst};
             // Initialize the routine options
             mIterations = itr;
             mInterval = intrv;
@@ -167,14 +160,22 @@ private:
                 if (SQ_FAILED(res))
                 {
                     // Should we endure the errors?
-                    if (!mEndure)
+                    if (!mEndure && !mTerminated)
                     {
-                        Terminate(); // Destroy ourself on error
+                        Terminate(); // Destroy our self on error
                     }
                 }
             }
+            // See if we are using the slot of a previously terminated routine
+            if  (mTerminated)
+            {
+                // The old routine was terminated, we're the new routine and we're not
+                // We don't touch the iterations and return the interval of the new routine
+                // to take effect on the next frame
+                mTerminated = false;
+            }
             // Decrease the number of iterations if necessary
-            if (mIterations && (--mIterations) == 0)
+            else if (mIterations && (--mIterations) == 0)
             {
                 Terminate(); // This routine reached the end of it's life
             }
@@ -203,6 +204,8 @@ private:
         {
             Release();
             Clear();
+            // Don't attempt to terminate this routine again
+            mTerminated = true;
         }
     };
 
@@ -260,6 +263,17 @@ protected:
 
 public:
 
+    /* --------------------------------------------------------------------------------------------
+     * Default constructor.
+    */
+    ~Routine()
+    {
+        if (m_Slot < SQMOD_MAX_ROUTINES)
+        {
+            Terminate();
+        }
+    }
+
     /* --------------------------------------------------------------------------------------------
      * Copy constructor. (disabled)
     */