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

Skip mem context cleanup in CATCH_FATAL() block.

Browse Source 
An error that gets raised all the way to the top TRY block might need to free a lot of resources and any of these callbacks could throw an error and mask the original error. In fact this is pretty likely since we are already in an error state. For example, the Db object will try to close the remote db connection, but if the protocol is in a bad state it will not be able to do so.

Solve this, for now, by not freeing memory or calling callbacks in the CATCH_FATAL() block. This gives us a better chance if being able to report the error without encountering another error first.

For the most part, we don't need to worry about freeing resources (file handles, TLS contexts, etc.) if the program is going to exit immediately. However, it is important to attempt to terminate all active protocol connections, which is done by protocolFree() in main() since the protocol objects live in the top context.

Another way to handle this would be to implement an error stack and that is probably something we will do in the future. But, in the case of a segfault the original error would still be lost. Yet another option would be to still do cleanup but defer it until after the CATCH_FATAL() block.
This commit is contained in:
David Steele 2022-08-31 12:44:55 -04:00 committed by GitHub
parent eda7f81ee4
commit 0f7b6a3344
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 61 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
doc/xml/release.xml
View File

@ -86,6 +86,17 @@
</release-improvement-list>
<release-development-list>
<release-item>
<github-pull-request id="1842"/>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="john.morris"/>
</release-item-contributor-list>
<p>Skip mem context cleanup in <code>CATCH_FATAL()</code> block.</p>
</release-item>
<release-item>
<github-pull-request id="1838"/>

2
src/common/error.c
View File

@ -308,7 +308,7 @@ errorInternalCatch(const ErrorType *const errorTypeCatch, const bool fatalCatch)
if (errorInternalState() == errorStateTry)
{
for (unsigned int handlerIdx = 0; handlerIdx < errorContext.handlerTotal; handlerIdx++)
errorContext.handlerList[handlerIdx](errorTryDepth());
errorContext.handlerList[handlerIdx](errorTryDepth(), fatalCatch);
errorContext.tryList[errorContext.tryTotal].state++;
}

8
src/common/error.h
View File

@ -122,7 +122,7 @@ Try stack getters/setters
unsigned int errorTryDepth(void);
// Add a handler to be called when an error occurs
typedef void (*const ErrorHandlerFunction)(unsigned int);
typedef void (*const ErrorHandlerFunction)(unsigned int, bool);
void errorHandlerSet(const ErrorHandlerFunction *list, unsigned int total);
@ -141,7 +141,7 @@ Begin a block where errors can be thrown
{
/***********************************************************************************************************************************
Catch a specific error thrown in the try block
Catch a specific error thrown in the try block. Fatal errors cannot be caught with this block.
***********************************************************************************************************************************/
#define CATCH(errorTypeCatch) \
} \
@ -157,7 +157,9 @@ Catch any non-fatal error thrown in the try block
{
/***********************************************************************************************************************************
Catch any error thrown in the try block
Catch any error thrown in the try block including fatal errors. To maximize the chance of being able to report the error caught,
memory contexts will not be freed and mem context callbacks will not be called. If the TRY block is enclosed in a mem context then
child mem contexts will be freed and child mem context callbacks called when the enclosing mem context is freed.
***********************************************************************************************************************************/
#define CATCH_FATAL() \
} \

9
src/common/memContext.c
View File

@ -1048,10 +1048,11 @@ memContextSize(const MemContext *const this)
/**********************************************************************************************************************************/
void
memContextClean(unsigned int tryDepth)
memContextClean(const unsigned int tryDepth, const bool fatal)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(UINT, tryDepth);
FUNCTION_TEST_PARAM(BOOL, false);
FUNCTION_TEST_END();
ASSERT(tryDepth > 0);
@ -1059,10 +1060,12 @@ memContextClean(unsigned int tryDepth)
// Iterate through everything pushed to the stack since the last try
while (memContextStack[memContextMaxStackIdx].tryDepth >= tryDepth)
{
// Free memory contexts that were not kept
// Free memory contexts that were not kept. Skip this for fatal errors to avoid calling destructors that could error and
// mask the original error.
if (memContextStack[memContextMaxStackIdx].type == memContextStackTypeNew)
{
memContextFree(memContextStack[memContextMaxStackIdx].memContext);
if (!fatal) // {uncovered !!!}
memContextFree(memContextStack[memContextMaxStackIdx].memContext);
}
// Else find the prior context and make it the current context
else

2
src/common/memContext.h
View File

@ -299,6 +299,6 @@ Macros for function logging
Internal functions
***********************************************************************************************************************************/
// Clean up mem contexts after an error. Should only be called from error handling routines.
void memContextClean(unsigned int tryDepth);
void memContextClean(unsigned int tryDepth, bool fatal);
#endif

4
src/common/stackTrace.c
View File

@ -348,8 +348,10 @@ stackTraceToZ(char *buffer, size_t bufferSize, const char *fileName, const char
/**********************************************************************************************************************************/
void
stackTraceClean(unsigned int tryDepth)
stackTraceClean(const unsigned int tryDepth, const bool fatal)
{
(void)fatal; // Cleanup is the same for fatal errors
while (stackTraceLocal.stackSize > 0 && stackTraceLocal.stack[stackTraceLocal.stackSize - 1].tryDepth >= tryDepth)
stackTraceLocal.stackSize--;
}

2
src/common/stackTrace.h
View File

@ -75,6 +75,6 @@ char *stackTraceParamBuffer(const char *param);
void stackTraceParamAdd(size_t bufferSize);
// Clean the stack at and below the try level. Called by the error to cleanup the stack when an exception occurs.
void stackTraceClean(unsigned int tryDepth);
void stackTraceClean(unsigned int tryDepth, bool fatal);
#endif

37
test/src/common/harnessTest.h
View File

@ -69,6 +69,17 @@ unsigned int testIdx(void);
/***********************************************************************************************************************************
Test that an expected error is actually thrown and error when it isn't
***********************************************************************************************************************************/
// Wrap the error in a temp mem context (when available) to free memory and execute callbacks after CATCH_FATAL()
#ifdef HRN_FEATURE_MEMCONTEXT
#include "common/memContext.h"
#define TEST_ERROR_MEM_CONTEXT_BEGIN() MEM_CONTEXT_TEMP_BEGIN()
#define TEST_ERROR_MEM_CONTEXT_END() MEM_CONTEXT_TEMP_END()
#else
#define TEST_ERROR_MEM_CONTEXT_BEGIN()
#define TEST_ERROR_MEM_CONTEXT_END()
#endif
#define TEST_ERROR(statement, errorTypeExpected, errorMessageExpected) \
{ \
bool TEST_ERROR_catch = false; \
@ -80,20 +91,24 @@ Test that an expected error is actually thrown and error when it isn't
printf("expect %s: %s\n", errorTypeName(&errorTypeExpected), errorMessageExpected); \
fflush(stdout); \
\
TRY_BEGIN() \
TEST_ERROR_MEM_CONTEXT_BEGIN() \
{ \
statement; \
} \
CATCH_FATAL() \
{ \
TEST_ERROR_catch = true; \
TRY_BEGIN() \
{ \
statement; \
} \
CATCH_FATAL() \
{ \
TEST_ERROR_catch = true; \
\
if (strcmp(errorMessage(), errorMessageExpected) != 0 || errorType() != &errorTypeExpected) \
THROW_FMT( \
TestError, "EXPECTED %s: %s\n\n BUT GOT %s: %s\n\nTHROWN AT:\n%s", errorTypeName(&errorTypeExpected), \
errorMessageExpected, errorName(), errorMessage(), errorStackTrace()); \
if (strcmp(errorMessage(), errorMessageExpected) != 0 || errorType() != &errorTypeExpected) \
THROW_FMT( \
TestError, "EXPECTED %s: %s\n\n BUT GOT %s: %s\n\nTHROWN AT:\n%s", errorTypeName(&errorTypeExpected), \
errorMessageExpected, errorName(), errorMessage(), errorStackTrace()); \
} \
TRY_END(); \
} \
TRY_END(); \
TEST_ERROR_MEM_CONTEXT_END(); \
\
if (!TEST_ERROR_catch) \
THROW_FMT( \

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

@ -48,10 +48,12 @@ testTryRecurse(void)
Test error handler
***********************************************************************************************************************************/
static unsigned int testErrorHandlerTryDepth;
static bool testErrorHandlerFatal;
static void
testErrorHandler(unsigned int tryDepth)
testErrorHandler(unsigned int tryDepth, bool fatal)
{
testErrorHandlerFatal = fatal;
testErrorHandlerTryDepth = tryDepth;
}
@ -184,6 +186,7 @@ testRun(void)
CATCH_FATAL()
{
assert(testErrorHandlerTryDepth == 3);
assert(testErrorHandlerFatal);
// Change to FormatError so error can be caught by normal catches
THROW(FormatError, errorMessage());
@ -198,6 +201,7 @@ testRun(void)
CATCH_ANY()
{
assert(testErrorHandlerTryDepth == 2);
assert(!testErrorHandlerFatal);
RETHROW();
}
@ -210,6 +214,7 @@ testRun(void)
CATCH(RuntimeError)
{
assert(testErrorHandlerTryDepth == 1);
assert(!testErrorHandlerFatal);
assert(errorTryDepth() == 1);
assert(errorContext.tryList[1].state == errorStateEnd);
assert(strlen(errorMessage()) == sizeof(messageBuffer) - 1);

2
test/src/test.c
View File

@ -238,7 +238,7 @@ main(int argListSize, const char *argList[])
#ifdef HRN_FEATURE_ERROR
}
CATCH_ANY()
CATCH_FATAL()
{
// If a test was running then throw a detailed result exception
#ifdef DEBUG