db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-07-13 01:00:23 +02:00
Code Issues Releases Activity

Simplify error handler.

Browse Source 
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.
This commit is contained in:
David Steele
2021-11-03 10:36:31 -04:00
committed by GitHub
parent cff961ede7
commit c5b5b58806
5 changed files with 73 additions and 92 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
doc/xml/release.xml
View File

@ -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"/>

15
src/command/backup/backup.c
View File

@ -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();
}
}

89
src/common/error.c
View File

@ -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
errorInternalStateTry(void)
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");
/**********************************************************************************************************************************/
bool
errorInternalStateCatch(const ErrorType *errorTypeCatch)
{
if (errorInternalState() == errorStateCatch && errorInstanceOf(errorTypeCatch))
return errorInternalProcess(true);
// Increment try total
errorContext.tryTotal++;
return false;
}
/**********************************************************************************************************************************/
bool
errorInternalStateFinal(void)
{
return errorInternalState() == errorStateFinal;
// Setup try
errorContext.tryList[errorContext.tryTotal].state = errorStateTry;
errorContext.tryList[errorContext.tryTotal].uncaught = false;
}
/**********************************************************************************************************************************/
@ -292,23 +284,28 @@ 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 (errorContext.tryTotal >= ERROR_TRY_MAX)
errorInternalThrowFmt(&AssertError, fileName, functionName, fileLine, "too many nested try blocks");
// 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());
// Increment try total
errorContext.tryTotal++;
errorContext.tryList[errorContext.tryTotal].state++;
}
// Setup try
errorContext.tryList[errorContext.tryTotal].state = errorStateBegin;
if (errorInternalState() == errorStateCatch && errorInstanceOf(errorTypeCatch))
{
errorContext.tryList[errorContext.tryTotal].uncaught = false;
errorContext.tryList[errorContext.tryTotal].state++;
// Try setup was successful
return true;
}
return false;
}
/**********************************************************************************************************************************/
void
errorInternalPropagate(void)
@ -331,52 +328,22 @@ errorInternalPropagate(void)
}
/**********************************************************************************************************************************/
bool
errorInternalProcess(bool catch)
void
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;
}
/**********************************************************************************************************************************/

40
src/common/error.h
View File

@ -126,37 +126,43 @@ Begin a block where errors can be thrown
#define TRY_BEGIN() \
do \
{ \
if (errorInternalTry(__FILE__, __func__, __LINE__) && setjmp(*errorInternalJump()) >= 0) \
{ \
while (errorInternalProcess(false)) \
{ \
if (errorInternalStateTry())
errorInternalTryBegin(__FILE__, __func__, __LINE__); \
\
if (setjmp(*errorInternalJump()) == 0) \
{
/***********************************************************************************************************************************
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() \
} \
\
errorInternalTryEnd(); \
} \
} while (0)
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);
// True when in final state
bool errorInternalStateFinal(void);
// Process the error through each try and state
bool errorInternalProcess(bool catch);
bool errorInternalCatch(const ErrorType *errorTypeCatch);
// 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,

4
test/src/module/common/errorTest.c
View File

@ -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;