db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-01-18 04:58:51 +02:00
Code Issues Releases Activity

Use variable instead of function to track FINALLY() state.

Browse Source 
The function worked fine, but Coverity was unable to determine that the finally block was run, which led to false positives about unfreed memory.

Using a boolean in the block makes it clear to Coverity that the finally block will always be run no matter what else happens.

We'll depend on the compiler to optimize away the boolean if it is not used in a finally block. The cost of the boolean is fairly low in comparison to everything else being done in these macros, so it does not seem worth having a separate block even if the compiler is not able to eliminate the boolean.

This reverts most of 9a271e9 that fixed a bug caused by c5b5b58, which was also attempting to help Coverity understand FINALLY() blocks.
This commit is contained in:
David Steele 2022-05-09 10:39:43 -04:00 committed by GitHub
parent e8c40a24df
commit ef4c4ab852
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 17 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
doc/xml/release.xml
View File

@ -22,6 +22,9 @@
<github-pull-request id="1732"/>
</commit>
<commit subject="Remove unnecessary TRY() block in common/regExp module."/>
<commit subject="Use variable instead of function to track FINALLY() state.">
<github-pull-request id="1744"/>
</commit>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>

45
src/common/error.c
View File

@ -44,7 +44,7 @@ Maximum allowed number of nested try blocks
/***********************************************************************************************************************************
States for each try
***********************************************************************************************************************************/
typedef enum {errorStateTry, errorStateCatch, errorStateFinally, errorStateEnd} ErrorState;
typedef enum {errorStateTry, errorStateCatch, errorStateEnd} ErrorState;
/***********************************************************************************************************************************
Track error handling
@ -298,16 +298,6 @@ 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)
@ -315,7 +305,9 @@ errorInternalCatch(const ErrorType *const errorTypeCatch, const bool fatalCatch)
// If just entering error state clean up the stack
if (errorInternalState() == errorStateTry)
{
errorInternalHandlerClean();
for (unsigned int handlerIdx = 0; handlerIdx < errorContext.handlerTotal; handlerIdx++)
errorContext.handlerList[handlerIdx](errorTryDepth());
errorContext.tryList[errorContext.tryTotal].state++;
}
@ -351,39 +343,12 @@ 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 (errorInternalState() == errorStateFinally && !errorContext.tryList[errorContext.tryTotal].uncaught)
if (errorInternalState() == errorStateEnd && !errorContext.tryList[errorContext.tryTotal].uncaught)
errorContext.error = (Error){0};
// Remove the try

11
src/common/error.h
View File

@ -132,6 +132,9 @@ Begin a block where errors can be thrown
#define TRY_BEGIN() \
do \
{ \
volatile bool TRY_finally = false; \
(void)TRY_finally; /* Will be unused if there is no finally block */ \
\
errorInternalTryBegin(__FILE__, __func__, __LINE__); \
\
if (setjmp(*errorInternalJump()) == 0) \
@ -167,8 +170,9 @@ Code to run whether the try block was successful or not
#define FINALLY() \
} \
\
if (errorInternalFinally()) \
{
if (!TRY_finally) \
{ \
TRY_finally = true;
/***********************************************************************************************************************************
End the try block
@ -312,9 +316,6 @@ 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);

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

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