Fix error thrown from FINALLY() causing an infinite loop.

Any error thrown resets execution to the last setjmp(), which means that parts of the try block need to make sure they don't get run again. FINALLY() was not doing this so if it threw an error it would end up back in the FINALLY() block, where the error would likely be thrown again, causing an infinite loop.

Fix this by tracking the state of FINALLY() and only running it once. This requires cleaning the error stack like CATCH*() and clearing the error like TRY_END() depending on the order of execution.

doc/xml/release.xml

@@ -17,6 +17,17 @@
         <release date="XXXX-XX-XX" version="2.39dev" title="UNDER DEVELOPMENT">
             <release-core-list>
                 <release-bug-list>
+                    <release-item>
+                        <github-pull-request id="1732"/>
+
+                        <release-item-contributor-list>
+                            <release-item-contributor id="david.steele"/>
+                            <release-item-reviewer id="stephen.frost"/>
+                        </release-item-contributor-list>
+
+                        <p>Fix error thrown from <code>FINALLY()</code> causing an infinite loop.</p>
+                    </release-item>
+
                     <release-item>
                         <github-issue id="1722"/>
                         <github-pull-request id="1730"/>

src/common/error.c

@@ -44,7 +44,7 @@ Maximum allowed number of nested try blocks
 /***********************************************************************************************************************************
 States for each try
 ***********************************************************************************************************************************/
-typedef enum {errorStateTry, errorStateCatch, errorStateEnd} ErrorState;
+typedef enum {errorStateTry, errorStateCatch, errorStateFinally, errorStateEnd} ErrorState;
 
 /***********************************************************************************************************************************
 Track error handling
@@ -298,6 +298,16 @@ errorInternalJump(void)
     return &errorContext.jumpList[errorContext.tryTotal - 1];
 }
 
+/***********************************************************************************************************************************
+Clean the error stack
+***********************************************************************************************************************************/
+static void
+errorInternalHandlerClean(void)
+{
+    for (unsigned int handlerIdx = 0; handlerIdx < errorContext.handlerTotal; handlerIdx++)
+        errorContext.handlerList[handlerIdx](errorTryDepth());
+}
+
 /**********************************************************************************************************************************/
 bool
 errorInternalCatch(const ErrorType *const errorTypeCatch, const bool fatalCatch)
@@ -305,9 +315,7 @@ errorInternalCatch(const ErrorType *const errorTypeCatch, const bool fatalCatch)
     // If just entering error state clean up the stack
     if (errorInternalState() == errorStateTry)
     {
-        for (unsigned int handlerIdx = 0; handlerIdx < errorContext.handlerTotal; handlerIdx++)
-            errorContext.handlerList[handlerIdx](errorTryDepth());
-
+        errorInternalHandlerClean();
         errorContext.tryList[errorContext.tryTotal].state++;
     }
 
@@ -343,16 +351,40 @@ errorInternalPropagate(void)
     exit(UnhandledError.code);
 }
 
+/**********************************************************************************************************************************/
+bool
+errorInternalFinally(void)
+{
+    // If finally has not already been processed
+    if (errorInternalState() < errorStateEnd)
+    {
+        // If just entering error state clean up the stack
+        if (errorInternalState() == errorStateTry)
+        {
+            errorInternalHandlerClean();
+        }
+        // Else any catch blocks have been processed and none of them called RETHROW() so clear the error
+        else if (errorInternalState() == errorStateFinally && !errorContext.tryList[errorContext.tryTotal].uncaught)
+            errorContext.error = (Error){0};
+
+        // Advance to end state
+        errorContext.tryList[errorContext.tryTotal].state += errorStateEnd - errorContext.tryList[errorContext.tryTotal].state;
+
+        // Process finally block
+        return true;
+    }
+
+    // Skip finally block
+    return false;
+}
+
 /**********************************************************************************************************************************/
 void
 errorInternalTryEnd(void)
 {
     // Any catch blocks have been processed and none of them called RETHROW() so clear the error
-    if (errorContext.tryList[errorContext.tryTotal].state == errorStateEnd &&
-        !errorContext.tryList[errorContext.tryTotal].uncaught)
-    {
+    if (errorInternalState() == errorStateFinally && !errorContext.tryList[errorContext.tryTotal].uncaught)
         errorContext.error = (Error){0};
-    }
 
     // Remove the try
     errorContext.tryTotal--;

src/common/error.h

@@ -166,6 +166,8 @@ Code to run whether the try block was successful or not
 ***********************************************************************************************************************************/
 #define FINALLY()                                                                                                                  \
         }                                                                                                                          \
+                                                                                                                                   \
+        if (errorInternalFinally())                                                                                                \
         {
 
 /***********************************************************************************************************************************
@@ -310,6 +312,9 @@ bool errorInternalCatch(const ErrorType *errorTypeCatch, bool fatalCatch);
 // Propagate the error up so it can be caught
 void errorInternalPropagate(void) __attribute__((__noreturn__));
 
+// Process finally block
+bool errorInternalFinally(void);
+
 // End the try block
 void errorInternalTryEnd(void);
 

test/src/module/common/errorTest.c

@@ -96,7 +96,7 @@ testRun(void)
         }
         FINALLY()
         {
-            assert(errorContext.tryList[1].state == errorStateTry);
+            assert(errorContext.tryList[1].state == errorStateEnd);
             finallyDone = true;
         }
         TRY_END();
@@ -157,12 +157,22 @@ testRun(void)
                     TRY_BEGIN()
                     {
                         assert(errorTryDepth() == 4);
-                        tryDone = true;
 
-                        char bigMessage[sizeof(messageBuffer) + 128];
-                        memset(bigMessage, 'A', sizeof(bigMessage));
+                        TRY_BEGIN()
+                        {
+                            assert(errorTryDepth() == 5);
+                            tryDone = true;
+                        }
+                        FINALLY()
+                        {
+                            assert(errorContext.tryList[5].state == errorStateEnd);
 
-                        THROW(AssertError, bigMessage);
+                            char bigMessage[sizeof(messageBuffer) + 128];
+                            memset(bigMessage, 'A', sizeof(bigMessage));
+
+                            THROW(AssertError, bigMessage);
+                        }
+                        TRY_END();
                     }
                     CATCH_ANY()
                     {
@@ -200,7 +210,7 @@ testRun(void)
         {
             assert(testErrorHandlerTryDepth == 1);
             assert(errorTryDepth() == 1);
-            assert(errorContext.tryList[1].state == errorStateEnd);
+            assert(errorContext.tryList[1].state == errorStateFinally);
             assert(strlen(errorMessage()) == sizeof(messageBuffer) - 1);
 
             catchDone = true;