Simplify error handler.

The error handler used a loop to process try, catch, and finally blocks. This worked fine but static analysis tools like Coverity did not understand that the finally block would always run and so there were false positives about double-free, unfreed resource, etc.

This implementation removes the loop, which simplifies everything, and makes it clear that the finally block will always run. This cuts down on Coverity false positives.

This implementation also catches lack of coverage on empty catch blocks so a few test fixes were committed separately in d74fe7a.

A small refactor in backup.c is required because gcc 10.3.1 on Fedora 33 complains that the reason variable may be used uninitialized. It's not clear why this is the case, but reducing the scope of the TRY block fixes the issue.

doc/xml/release.xml

@@ -17,6 +17,17 @@
         <release date="XXXX-XX-XX" version="2.37dev" title="UNDER DEVELOPMENT">
             <release-core-list>
                 <release-development-list>
+                    <release-item>
+                        <github-pull-request id="1549"/>
+
+                        <release-item-contributor-list>
+                            <release-item-contributor id="david.steele"/>
+                            <release-item-reviewer id="stephen.frost"/>
+                        </release-item-contributor-list>
+
+                        <p>Simplify error handler.</p>
+                    </release-item>
+
                     <release-item>
                         <commit subject="Simplify strIdFrom*() functions.">
                             <github-pull-request id="1551"/>

src/command/backup/backup.c

@@ -666,12 +666,19 @@ backupResumeFind(const Manifest *manifest, const String *cipherPassBackup)
                     // occurs then the backup will be considered unusable and a resume will not be attempted.
                     if (cfgOptionBool(cfgOptResume))
                     {
-                        reason = strNewFmt("unable to read %s" INFO_COPY_EXT, strZ(manifestFile));
-
                         TRY_BEGIN()
                         {
                             manifestResume = manifestLoadFile(
                                 storageRepo(), manifestFile, cfgOptionStrId(cfgOptRepoCipherType), cipherPassBackup);
+                        }
+                        CATCH_ANY()
+                        {
+                            reason = strNewFmt("unable to read %s" INFO_COPY_EXT, strZ(manifestFile));
+                        }
+                        TRY_END();
+
+                        if (manifestResume != NULL)
+                        {
                             const ManifestData *manifestResumeData = manifestData(manifestResume);
 
                             // Check pgBackRest version. This allows the resume implementation to be changed with each version of
@@ -712,10 +719,6 @@ backupResumeFind(const Manifest *manifest, const String *cipherPassBackup)
                             else
                                 usable = true;
                         }
-                        CATCH_ANY()
-                        {
-                        }
-                        TRY_END();
                     }
                 }
 

src/common/error.c

@@ -42,7 +42,7 @@ Maximum allowed number of nested try blocks
 /***********************************************************************************************************************************
 States for each try
 ***********************************************************************************************************************************/
-typedef enum {errorStateBegin, errorStateTry, errorStateCatch, errorStateFinal, errorStateEnd} ErrorState;
+typedef enum {errorStateTry, errorStateCatch, errorStateEnd} ErrorState;
 
 /***********************************************************************************************************************************
 Track error handling
@@ -260,27 +260,19 @@ errorInternalState(void)
 }
 
 /**********************************************************************************************************************************/
-bool
+void
-errorInternalStateTry(void)
+errorInternalTryBegin(const char *const fileName, const char *const functionName, const int fileLine)
 {
-    return errorInternalState() == errorStateTry;
+    // If try total has been exceeded then throw an error
-}
+    if (errorContext.tryTotal >= ERROR_TRY_MAX)
+        errorInternalThrowFmt(&AssertError, fileName, functionName, fileLine, "too many nested try blocks");
 
-/**********************************************************************************************************************************/
+    // Increment try total
-bool
+    errorContext.tryTotal++;
-errorInternalStateCatch(const ErrorType *errorTypeCatch)
-{
-    if (errorInternalState() == errorStateCatch && errorInstanceOf(errorTypeCatch))
-        return errorInternalProcess(true);
 
-    return false;
+    // Setup try
-}
+    errorContext.tryList[errorContext.tryTotal].state = errorStateTry;
-
+    errorContext.tryList[errorContext.tryTotal].uncaught = false;
-/**********************************************************************************************************************************/
-bool
-errorInternalStateFinal(void)
-{
-    return errorInternalState() == errorStateFinal;
 }
 
 /**********************************************************************************************************************************/
@@ -292,21 +284,26 @@ errorInternalJump(void)
 
 /**********************************************************************************************************************************/
 bool
-errorInternalTry(const char *fileName, const char *functionName, int fileLine)
+errorInternalCatch(const ErrorType *const errorTypeCatch)
 {
-    // If try total has been exceeded then throw an error
+    // If just entering error state clean up the stack
-    if (errorContext.tryTotal >= ERROR_TRY_MAX)
+    if (errorInternalState() == errorStateTry)
-        errorInternalThrowFmt(&AssertError, fileName, functionName, fileLine, "too many nested try blocks");
+    {
+        for (unsigned int handlerIdx = 0; handlerIdx < errorContext.handlerTotal; handlerIdx++)
+            errorContext.handlerList[handlerIdx](errorTryDepth());
 
-    // Increment try total
+        errorContext.tryList[errorContext.tryTotal].state++;
-    errorContext.tryTotal++;
+    }
 
-    // Setup try
+    if (errorInternalState() == errorStateCatch && errorInstanceOf(errorTypeCatch))
-    errorContext.tryList[errorContext.tryTotal].state = errorStateBegin;
+    {
-    errorContext.tryList[errorContext.tryTotal].uncaught = false;
+        errorContext.tryList[errorContext.tryTotal].uncaught = false;
+        errorContext.tryList[errorContext.tryTotal].state++;
 
-    // Try setup was successful
+        return true;
-    return true;
+    }
+
+    return false;
 }
 
 /**********************************************************************************************************************************/
@@ -331,52 +328,22 @@ errorInternalPropagate(void)
 }
 
 /**********************************************************************************************************************************/
-bool
+void
-errorInternalProcess(bool catch)
+errorInternalTryEnd(void)
 {
-    // If a catch statement then return
-    if (catch)
-    {
-        errorContext.tryList[errorContext.tryTotal].uncaught = false;
-        return true;
-    }
-    // Else if just entering error state clean up the stack
-    else if (errorContext.tryList[errorContext.tryTotal].state == errorStateTry)
-    {
-        for (unsigned int handlerIdx = 0; handlerIdx < errorContext.handlerTotal; handlerIdx++)
-            errorContext.handlerList[handlerIdx](errorTryDepth());
-    }
-
     // Any catch blocks have been processed and none of them called RETHROW() so clear the error
-    if (errorContext.tryList[errorContext.tryTotal].state == errorStateCatch &&
+    if (errorContext.tryList[errorContext.tryTotal].state == errorStateEnd &&
         !errorContext.tryList[errorContext.tryTotal].uncaught)
     {
         errorContext.error = (Error){0};
     }
 
-    // Increment the state
-    errorContext.tryList[errorContext.tryTotal].state++;
-
-    // If the error has been caught then increment the state
-    if (errorContext.tryList[errorContext.tryTotal].state == errorStateCatch &&
-        !errorContext.tryList[errorContext.tryTotal].uncaught)
-    {
-        errorContext.tryList[errorContext.tryTotal].state++;
-    }
-
-    // Return if not done
-    if (errorContext.tryList[errorContext.tryTotal].state < errorStateEnd)
-        return true;
-
     // Remove the try
     errorContext.tryTotal--;
 
     // If not caught in the last try then propagate
     if (errorContext.tryList[errorContext.tryTotal + 1].uncaught)
         errorInternalPropagate();
-
-    // Nothing left to process
-    return false;
 }
 
 /**********************************************************************************************************************************/

src/common/error.h

@@ -126,37 +126,43 @@ Begin a block where errors can be thrown
 #define TRY_BEGIN()                                                                                                                \
     do                                                                                                                             \
     {                                                                                                                              \
-        if (errorInternalTry(__FILE__, __func__, __LINE__) && setjmp(*errorInternalJump()) >= 0)                                   \
+        errorInternalTryBegin(__FILE__, __func__, __LINE__);                                                                       \
-        {                                                                                                                          \
+                                                                                                                                   \
-            while (errorInternalProcess(false))                                                                                    \
+        if (setjmp(*errorInternalJump()) == 0)                                                                                     \
-            {                                                                                                                      \
+        {
-                if (errorInternalStateTry())
 
 /***********************************************************************************************************************************
 Catch a specific error thrown in the try block
 ***********************************************************************************************************************************/
 #define CATCH(errorTypeCatch)                                                                                                      \
-                else if (errorInternalStateCatch(&errorTypeCatch))
+        }                                                                                                                          \
+        else if (errorInternalCatch(&errorTypeCatch))                                                                              \
+        {
 
 /***********************************************************************************************************************************
 Catch any error thrown in the try block
 ***********************************************************************************************************************************/
 #define CATCH_ANY()                                                                                                                \
-                else if (errorInternalStateCatch(&RuntimeError))
+        }                                                                                                                          \
+        else if (errorInternalCatch(&RuntimeError))                                                                                \
+        {
 
 /***********************************************************************************************************************************
 Code to run whether the try block was successful or not
 ***********************************************************************************************************************************/
 #define FINALLY()                                                                                                                  \
-                else if (errorInternalStateFinal())
+        }                                                                                                                          \
+        {
 
 /***********************************************************************************************************************************
 End the try block
 ***********************************************************************************************************************************/
 #define TRY_END()                                                                                                                  \
-            }                                                                                                                      \
         }                                                                                                                          \
-    } while (0)
+                                                                                                                                   \
+        errorInternalTryEnd();                                                                                                     \
+    }                                                                                                                              \
+    while (0)
 
 /***********************************************************************************************************************************
 Throw an error
@@ -279,26 +285,20 @@ Internal functions
 These functions are used by the macros to implement the error handler and should never be called independently.
 ***********************************************************************************************************************************/
 // Begin the try block
-bool errorInternalTry(const char *fileName, const char *functionName, int fileLine);
+void errorInternalTryBegin(const char *fileName, const char *functionName, int fileLine);
 
 // Return jump buffer for current try
 jmp_buf *errorInternalJump(void);
 
-// True when in try state
-bool errorInternalStateTry(void);
-
 // True when in catch state and the expected error matches
-bool errorInternalStateCatch(const ErrorType *errorTypeCatch);
+bool errorInternalCatch(const ErrorType *errorTypeCatch);
-
-// True when in final state
-bool errorInternalStateFinal(void);
-
-// Process the error through each try and state
-bool errorInternalProcess(bool catch);
 
 // Propagate the error up so it can be caught
 void errorInternalPropagate(void) __attribute__((__noreturn__));
 
+// End the try block
+void errorInternalTryEnd(void);
+
 // Throw an error
 void errorInternalThrow(
     const ErrorType *errorType, const char *fileName, const char *functionName, int fileLine, const char *message,

test/src/module/common/errorTest.c

@@ -96,7 +96,7 @@ testRun(void)
         }
         FINALLY()
         {
-            assert(errorContext.tryList[1].state == errorStateFinal);
+            assert(errorContext.tryList[1].state == errorStateTry);
             finallyDone = true;
         }
         TRY_END();
@@ -195,7 +195,7 @@ testRun(void)
         {
             assert(testErrorHandlerTryDepth == 1);
             assert(errorTryDepth() == 1);
-            assert(errorContext.tryList[1].state == errorStateCatch);
+            assert(errorContext.tryList[1].state == errorStateEnd);
             assert(strlen(errorMessage()) == sizeof(messageBuffer) - 1);
 
             catchDone = true;