Do not allow CATCH() to catch a fatal error.

Catching individual fatal errors was only used in testing so the tests have been updated to use other errors instead. CATCH_FATAL() is now the only way to catch fatal errors.

This simplifies the logic a bit for upcoming changes to error handling and cleanup.

Also fix an issue where passing errorMessage() directly to THROW*() would attempt to copy the message buffer instead of preserving it, which is undefined behavior. Since there were no instances of this behavior before this commit, this was not a live bug.

doc/xml/release.xml

@@ -41,6 +41,19 @@
                         <p>Improve warning message on backup resume.</p>
                     </release-item>
                 </release-improvement-list>
+
+                <release-development-list>
+                    <release-item>
+                        <github-pull-request id="1838"/>
+
+                        <release-item-contributor-list>
+                            <release-item-contributor id="david.steele"/>
+                            <release-item-reviewer id="john.morris"/>
+                        </release-item-contributor-list>
+
+                        <p>Do not allow <code>CATCH()</code> to catch a fatal error.</p>
+                    </release-item>
+                </release-development-list>
             </release-core-list>
 
             <release-doc-list>

src/common/error.c

@@ -302,6 +302,8 @@ errorInternalJump(void)
 bool
 errorInternalCatch(const ErrorType *const errorTypeCatch, const bool fatalCatch)
 {
+    assert(fatalCatch || !errorTypeFatal(errorTypeCatch));
+
     // If just entering error state clean up the stack
     if (errorInternalState() == errorStateTry)
     {
@@ -371,9 +373,12 @@ errorInternalThrow(
     errorContext.error.functionName = functionName;
     errorContext.error.fileLine = fileLine;
 
-    // Assign message to the error
-    strncpy(messageBuffer, message, sizeof(messageBuffer));
-    messageBuffer[sizeof(messageBuffer) - 1] = 0;
+    // Assign message to the error. If errorMessage() is passed as the message there is no need to make a copy.
+    if (message != messageBuffer)
+    {
+        strncpy(messageBuffer, message, sizeof(messageBuffer));
+        messageBuffer[sizeof(messageBuffer) - 1] = 0;
+    }
 
     errorContext.error.message = (const char *)messageBuffer;
 

src/common/error.h

@@ -145,7 +145,7 @@ Catch a specific error thrown in the try block
 ***********************************************************************************************************************************/
 #define CATCH(errorTypeCatch)                                                                                                      \
         }                                                                                                                          \
-        else if (errorInternalCatch(&errorTypeCatch, true))                                                                        \
+        else if (errorInternalCatch(&errorTypeCatch, false))                                                                       \
         {
 
 /***********************************************************************************************************************************

test/src/module/common/errorTest.c

@@ -33,7 +33,7 @@ testTryRecurse(void)
 
         testTryRecurse();
     }
-    CATCH(MemoryError)
+    CATCH(UnhandledError)
     {
         testTryRecurseCatch = true;                                 // {uncoverable - catch should never be executed}
     }
@@ -181,20 +181,21 @@ testRun(void)
                     }
                     TRY_END();
                 }
-                CATCH(AssertError)
+                CATCH_FATAL()
                 {
                     assert(testErrorHandlerTryDepth == 3);
 
-                    // Finally below should run even though this error has been rethrown
-                    RETHROW();
+                    // Change to FormatError so error can be caught by normal catches
+                    THROW(FormatError, errorMessage());
                 }
+                // Should run even though an error has been thrown in the catch
                 FINALLY()
                 {
                     finallyDone = true;
                 }
                 TRY_END();
             }
-            CATCH_FATAL()
+            CATCH_ANY()
             {
                 assert(testErrorHandlerTryDepth == 2);
 
@@ -202,7 +203,7 @@ testRun(void)
             }
             TRY_END();
         }
-        CATCH(MemoryError)
+        CATCH(UnhandledError)
         {
             assert(false);                                              // {uncoverable - catch should never be executed}
         }
@@ -238,7 +239,7 @@ testRun(void)
             tryDone = true;
             testTryRecurse();
         }
-        CATCH(AssertError)
+        CATCH_FATAL()
         {
             assert(errorCode() == AssertError.code);
             assert(strcmp(errorFileName(), TEST_PGB_PATH "/test/src/module/common/errorTest.c") == 0);

test/src/module/common/memContextTest.c

@@ -414,11 +414,11 @@ testRun(void)
             {
                 memContext = MEM_CONTEXT_NEW();
                 TEST_RESULT_Z(memContext->name, memContextTestName, "check mem context name");
-                THROW(AssertError, "create failed");
+                THROW(FormatError, "create failed");
             }
             MEM_CONTEXT_NEW_END();
         }
-        CATCH(AssertError)
+        CATCH(FormatError)
         {
             catch = true;
         }