db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-03-05 15:05:48 +02:00
Code Issues Releases Activity

Prevent memContextFree() from freeing memory needed by callbacks.

Browse Source 
The order of callbacks and frees meant that memory needed during a callback (for logging in all known cases) might end up being freed before a callback needed it.

Requiring callbacks and logging to check the validity of their allocations is pretty risky and it is not clear that all possible cases have been accounted for.

Instead recursively execute all the callbacks first and then come back and recursively free the context. This is safer and it removes the need to check if a context is freeing so a simple active flag (in debug builds) will do. The caller no longer needs this information at all so remove memContextFreeing() and objMemContextFreeing().
This commit is contained in:
David Steele 2022-05-04 14:53:05 -04:00 committed by GitHub
parent d9088b2e2b
commit ef672c74ad
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 136 additions and 133 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
doc/xml/release.xml
View File

@ -270,6 +270,17 @@
<p>Lock module refactoring.</p>
</release-item>
<release-item>
<github-pull-request id="1734"/>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="reid.thompson"/>
</release-item-contributor-list>
<p>Prevent <code>memContextFree()</code> from freeing memory needed by callbacks.</p>
</release-item>
<release-item>
<commit subject="Update postgres/client unit test for changes in libpq."/>
<commit subject="Update postgres/client unit test to conform to current patterns."/>

3
src/common/io/tls/client.c
View File

@ -48,8 +48,7 @@ tlsClientToLog(const THIS_VOID)
return strNewFmt(
"{ioClient: %s, timeoutConnect: %" PRIu64 ", timeoutSession: %" PRIu64 ", verifyPeer: %s}",
objMemContextFreeing(this) ? NULL_Z : strZ(ioClientToLog(this->ioClient)), this->timeoutConnect, this->timeoutSession,
cvtBoolToConstZ(this->verifyPeer));
strZ(ioClientToLog(this->ioClient)), this->timeoutConnect, this->timeoutSession, cvtBoolToConstZ(this->verifyPeer));
}
#define FUNCTION_LOG_TLS_CLIENT_TYPE \

3
src/common/io/tls/session.c
View File

@ -40,8 +40,7 @@ tlsSessionToLog(const THIS_VOID)
THIS(const TlsSession);
return strNewFmt(
"{ioSession: %s, timeout: %" PRIu64", shutdownOnClose: %s}",
objMemContextFreeing(this) ? NULL_Z : strZ(ioSessionToLog(this->ioSession)), this->timeout,
"{ioSession: %s, timeout: %" PRIu64", shutdownOnClose: %s}", strZ(ioSessionToLog(this->ioSession)), this->timeout,
cvtBoolToConstZ(this->shutdownOnClose));
}

218
src/common/memContext.c
View File

@ -10,15 +10,6 @@ Memory Context Manager
#include "common/error.h"
#include "common/memContext.h"
/***********************************************************************************************************************************
Memory context states
***********************************************************************************************************************************/
typedef enum
{
memContextStateFreeing = 0,
memContextStateActive
} MemContextState;
/***********************************************************************************************************************************
Contains information about a memory allocation. This header is placed at the beginning of every memory allocation returned to the
user by memNew(), etc. The advantage is that when an allocation is passed back by the user we know the location of the allocation
@ -51,8 +42,8 @@ struct MemContext
{
#ifdef DEBUG
const char *name; // Indicates what the context is being used for
bool active:1; // Is the context currently active?
#endif
MemContextState state:1; // Current state of the context
size_t allocExtra:16; // Size of extra allocation (1kB max)
unsigned int contextParentIdx; // Index in the parent context list
@ -78,9 +69,9 @@ generally used to allocate memory that exists for the life of the program.
***********************************************************************************************************************************/
static MemContext contextTop =
{
.state = memContextStateActive,
#ifdef DEBUG
.name = "TOP",
.active = true,
#endif
};
@ -295,13 +286,12 @@ memContextNew(
#ifdef DEBUG
// Set the context name
.name = name,
#endif
// Set extra allocation
.allocExtra = param.allocExtra,
// Set new context active
.state = memContextStateActive,
.active = true,
#endif
// Set extra allocation
.allocExtra = param.allocExtra,
// Set current context as the parent
.contextParent = contextCurrent,
@ -377,12 +367,9 @@ memContextCallbackSet(MemContext *this, void (*callbackFunction)(void *), void *
FUNCTION_TEST_END();
ASSERT(this != NULL);
ASSERT(this->active);
ASSERT(callbackFunction != NULL);
// Error if context is not active
if (this->state != memContextStateActive)
THROW(AssertError, "cannot assign callback to inactive context");
// Top context cannot have a callback
if (this == &contextTop)
THROW(AssertError, "top context may not have a callback");
@ -410,9 +397,6 @@ memContextCallbackClear(MemContext *this)
ASSERT(this != NULL);
// Error if context is not active or freeing
ASSERT(this->state == memContextStateActive || this->state == memContextStateFreeing);
// Top context cannot have a callback
ASSERT(this != &contextTop);
@ -611,12 +595,9 @@ memContextSwitch(MemContext *this)
FUNCTION_TEST_END();
ASSERT(this != NULL);
ASSERT(this->active);
ASSERT(memContextCurrentStackIdx < MEM_CONTEXT_STACK_MAX - 1);
// Error if context is not active
if (this->state != memContextStateActive)
THROW(AssertError, "cannot switch to inactive context");
memContextMaxStackIdx++;
memContextCurrentStackIdx = memContextMaxStackIdx;
@ -721,19 +702,6 @@ memContextCurrent(void)
FUNCTION_TEST_RETURN(MEM_CONTEXT, memContextStack[memContextCurrentStackIdx].memContext);
}
/**********************************************************************************************************************************/
bool
memContextFreeing(const MemContext *const this)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(MEM_CONTEXT, this);
FUNCTION_TEST_END();
ASSERT(this != NULL);
FUNCTION_TEST_RETURN(BOOL, this->state == memContextStateFreeing);
}
/**********************************************************************************************************************************/
MemContext *
memContextPrior(void)
@ -814,8 +782,9 @@ memContextClean(unsigned int tryDepth)
}
/**********************************************************************************************************************************/
void
memContextFree(MemContext *this)
// Helper to execute callbacks for the context and all its children
static void
memContextCallbackRecurse(MemContext *const this)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(MEM_CONTEXT, this);
@ -823,73 +792,114 @@ memContextFree(MemContext *this)
ASSERT(this != NULL);
// If context is already freeing then return if memContextFree() is called again - this can happen in callbacks
if (this->state != memContextStateFreeing)
{
#ifdef DEBUG
// Current context cannot be freed unless it is top (top is never really freed, just the stuff under it)
if (this == memContextStack[memContextCurrentStackIdx].memContext && this != &contextTop)
THROW_FMT(AssertError, "cannot free current context '%s'", this->name);
// Certain actions against the context are no longer allowed
this->active = false;
#endif
// Free child contexts
for (unsigned int contextIdx = 0; contextIdx < this->contextChildListSize; contextIdx++)
{
if (this->contextChildList[contextIdx] != NULL)
memContextFree(this->contextChildList[contextIdx]);
}
// Callback
if (this->callbackFunction)
{
this->callbackFunction(this->callbackArgument);
this->callbackFunction = NULL;
}
// Set state to freeing now that there are no child contexts. Child contexts might need to interact with their parent while
// freeing so the parent needs to remain active until they are all gone.
this->state = memContextStateFreeing;
// Execute callback if defined
TRY_BEGIN()
{
if (this->callbackFunction)
this->callbackFunction(this->callbackArgument);
}
// Finish cleanup even if the callback fails
FINALLY()
{
// Free child context allocation list
if (this->contextChildListSize > 0)
{
memFreeInternal(this->contextChildList);
this->contextChildListSize = 0;
}
// Free memory allocations and list
if (this->allocListSize > 0)
{
for (unsigned int allocIdx = 0; allocIdx < this->allocListSize; allocIdx++)
if (this->allocList[allocIdx] != NULL)
memFreeInternal(this->allocList[allocIdx]);
memFreeInternal(this->allocList);
this->allocListSize = 0;
}
// If the context index is lower than the current free index in the parent then replace it
if (this->contextParent != NULL && this->contextParentIdx < this->contextParent->contextChildFreeIdx)
this->contextParent->contextChildFreeIdx = this->contextParentIdx;
// Make top context active again
if (this == &contextTop)
{
this->state = memContextStateActive;
}
// Else free the memory context so the slot can be reused
else
{
ASSERT(this->contextParent != NULL);
this->contextParent->contextChildList[this->contextParentIdx] = NULL;
memFreeInternal(this);
}
}
TRY_END();
// Child callbacks
for (unsigned int contextIdx = 0; contextIdx < this->contextChildListSize; contextIdx++)
{
if (this->contextChildList[contextIdx] != NULL)
memContextCallbackRecurse(this->contextChildList[contextIdx]);
}
FUNCTION_TEST_RETURN_VOID();
}
// Helper to free the context and all its children
static void
memContextFreeRecurse(MemContext *const this)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(MEM_CONTEXT, this);
FUNCTION_TEST_END();
ASSERT(this != NULL);
#ifdef DEBUG
// Current context cannot be freed unless it is top (top is never really freed, just the stuff under it)
if (this == memContextStack[memContextCurrentStackIdx].memContext && this != &contextTop)
THROW_FMT(AssertError, "cannot free current context '%s'", this->name);
#endif
// Free child contexts and list
if (this->contextChildListSize > 0)
{
// Free child contexts
for (unsigned int contextIdx = 0; contextIdx < this->contextChildListSize; contextIdx++)
{
if (this->contextChildList[contextIdx] != NULL)
memContextFreeRecurse(this->contextChildList[contextIdx]);
}
// Free child context allocation list
memFreeInternal(this->contextChildList);
this->contextChildListSize = 0;
}
// Free memory allocations and list
if (this->allocListSize > 0)
{
for (unsigned int allocIdx = 0; allocIdx < this->allocListSize; allocIdx++)
if (this->allocList[allocIdx] != NULL)
memFreeInternal(this->allocList[allocIdx]);
memFreeInternal(this->allocList);
this->allocListSize = 0;
}
// If the context index is lower than the current free index in the parent then replace it
if (this->contextParent != NULL && this->contextParentIdx < this->contextParent->contextChildFreeIdx)
this->contextParent->contextChildFreeIdx = this->contextParentIdx;
// Free the memory context so the slot can be reused (if not the top mem context)
if (this != &contextTop)
{
ASSERT(this->contextParent != NULL);
this->contextParent->contextChildList[this->contextParentIdx] = NULL;
memFreeInternal(this);
}
#ifdef DEBUG
// Else make the top mem context active again
else
{
this->active = true;
}
#endif
FUNCTION_TEST_RETURN_VOID();
}
void
memContextFree(MemContext *const this)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(MEM_CONTEXT, this);
FUNCTION_TEST_END();
ASSERT(this != NULL);
ASSERT(this->active);
// Execute callbacks
TRY_BEGIN()
{
memContextCallbackRecurse(this);
}
// Free context even if a callback fails
FINALLY()
{
memContextFreeRecurse(this);
}
TRY_END();
FUNCTION_TEST_RETURN_VOID();
}

3
src/common/memContext.h
View File

@ -267,9 +267,6 @@ const MemContext *memContextConstFromAllocExtra(const void *allocExtra);
// Current memory context
MemContext *memContextCurrent(void);
// Is the mem context currently being freed?
bool memContextFreeing(const MemContext *this);
// Prior context, i.e. the context that was current before the last memContextSwitch()
MemContext *memContextPrior(void);

7
src/common/type/object.h
View File

@ -97,13 +97,6 @@ objMemContext(void *const this)
return memContextFromAllocExtra(this);
}
// Is the object mem context currently being freed?
__attribute__((always_inline)) static inline bool
objMemContextFreeing(const void *const this)
{
return memContextFreeing(memContextConstFromAllocExtra(this));
}
// Move an object to a new context if this != NULL
void *objMove(THIS_VOID, MemContext *parentNew);

23
test/src/module/common/memContextTest.c
View File

@ -13,12 +13,10 @@ testFree(void *thisVoid)
{
MemContext *this = thisVoid;
TEST_RESULT_BOOL(memContextFreeing(this), true, "state should be freeing before memContextFree() in callback");
memContextFree(this);
TEST_RESULT_INT(this->state, memContextStateFreeing, "state should still be freeing after memContextFree() in callback");
TEST_ERROR(memContextCallbackSet(this, testFree, this), AssertError, "cannot assign callback to inactive context");
TEST_ERROR(memContextSwitch(this), AssertError, "cannot switch to inactive context");
TEST_RESULT_BOOL(this->active, false, "mem context should be inactive while freeing");
TEST_ERROR(memContextFree(this), AssertError, "assertion 'this->active' failed");
TEST_ERROR(memContextCallbackSet(this, testFree, this), AssertError, "assertion 'this->active' failed");
TEST_ERROR(memContextSwitch(this), AssertError, "assertion 'this->active' failed");
memContextCallbackArgument = this;
@ -106,8 +104,7 @@ testRun(void)
memContextSwitch(memContextTop());
memContextNewP("test-filler");
memContextKeep();
TEST_RESULT_BOOL(
memContextTop()->contextChildList[contextIdx]->state == memContextStateActive, true, "new context is active");
TEST_RESULT_BOOL(memContextTop()->contextChildList[contextIdx]->active, true, "new context is active");
TEST_RESULT_Z(memContextTop()->contextChildList[contextIdx]->name, "test-filler", "new context name");
}
@ -130,8 +127,7 @@ testRun(void)
memContextNewP("test-reuse");
memContextKeep();
TEST_RESULT_BOOL(
memContextTop()->contextChildList[1]->state == memContextStateActive,
true, "new context in same index as freed context is active");
memContextTop()->contextChildList[1]->active, true, "new context in same index as freed context is active");
TEST_RESULT_Z(memContextTop()->contextChildList[1]->name, "test-reuse", "new context name");
TEST_RESULT_UINT(memContextTop()->contextChildFreeIdx, 2, "check context free idx");
@ -157,9 +153,8 @@ testRun(void)
memContextFree(memContextTop()->contextChildList[MEM_CONTEXT_INITIAL_SIZE]),
AssertError, "cannot free current context 'test5'");
memContextSwitch(memContextTop());
// memContextFree(memContextTop()->contextChildList[MEM_CONTEXT_INITIAL_SIZE]);
memContextFree(memContextTop());
TEST_RESULT_VOID(memContextSwitch(memContextTop()), "switch to top");
TEST_RESULT_VOID(memContextFree(memContextTop()), "free top");
MemContext *noAllocation = memContextNewP("empty");
memContextKeep();
@ -337,7 +332,7 @@ testRun(void)
TEST_RESULT_Z(memContextCurrent()->name, "TOP", "context name is now 'TOP'");
TEST_RESULT_PTR(memContextCurrent(), memContextTop(), "context is now 'TOP'");
TEST_RESULT_BOOL(memContext->state == memContextStateActive, true, "new mem context is still active");
TEST_RESULT_BOOL(memContext->active, true, "new mem context is still active");
memContextFree(memContext);
// ------------------------------------------------------------------------------------------------------------------------

1
test/src/module/common/typeObjectTest.c
View File

@ -107,7 +107,6 @@ testRun(void)
MEM_CONTEXT_TEMP_END();
TEST_RESULT_PTR(objMemContext(testObject), memContextFromAllocExtra(testObject), "mem context");
TEST_RESULT_BOOL(objMemContextFreeing(testObject), false, "not freeing");
TEST_RESULT_VOID(testObjectFree(testObject), "free object");
TEST_RESULT_VOID(testObjectFree(NULL), "free null object");