From a9feaba9e521e39928ff5fdbc025eeff8fc17646 Mon Sep 17 00:00:00 2001 From: David Steele Date: Wed, 7 Nov 2018 08:51:32 -0500 Subject: [PATCH] Add memContextCallbackClear() to prevent double free() calls. If an object free() method was called manually when a callback was set then the callback would call free() again. This meant that each free() method had to protect against a subsequent call. Instead, clear the callback (if present) before calling memContextFree(). This is faster (since there is no unecessary callback) and removes the need for semaphores to protect against a double free(). --- doc/xml/release.xml | 4 ++++ src/common/memContext.c | 26 +++++++++++++++++++++++++ src/common/memContext.h | 1 + src/common/regExp.c | 2 ++ src/compress/gzipCompress.c | 10 ++++------ src/compress/gzipDecompress.c | 9 +++------ src/crypto/cipherBlock.c | 1 + src/crypto/hash.c | 8 ++------ src/storage/driver/posix/fileRead.c | 2 ++ src/storage/driver/posix/fileWrite.c | 2 ++ test/src/module/common/memContextTest.c | 4 ++++ test/src/module/compress/gzipTest.c | 2 -- 12 files changed, 51 insertions(+), 20 deletions(-) diff --git a/doc/xml/release.xml b/doc/xml/release.xml index da073a84e..fdb167367 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -90,6 +90,10 @@

Change infoArchiveCheckPg() to display the version as a string (e.g. 9.4) instead of the integer representation (e.g. 90400) when throwing an error.

+ +

Add memContextCallbackClear() to prevent double free() calls.

+ +

Merge crypto/random module into crypto/crypto.

 diff --git a/src/common/memContext.c b/src/common/memContext.c index 7f9dbe94f..f4b7879f5 100644 --- a/src/common/memContext.c +++ b/src/common/memContext.c @@ -265,6 +265,32 @@ memContextCallback(MemContext *this, void (*callbackFunction)(void *), void *cal FUNCTION_TEST_RESULT_VOID(); } +/*********************************************************************************************************************************** +Clear the mem context callback. This is usually done in the object free method after resources have been freed but before +memContextFree() is called. The goal is to prevent the object free method from being called more than once. +***********************************************************************************************************************************/ +void +memContextCallbackClear(MemContext *this) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(MEM_CONTEXT, this); + + FUNCTION_TEST_ASSERT(this != NULL); + FUNCTION_TEST_END(); + + // Error if context is not active or freeing + ASSERT(this->state == memContextStateActive || this->state == memContextStateFreeing); + + // Top context cannot have a callback + ASSERT(this != memContextTop()); + + // Clear callback function and argument + this->callbackFunction = NULL; + this->callbackArgument = NULL; + + FUNCTION_TEST_RESULT_VOID(); +} + /*********************************************************************************************************************************** Allocate memory in the memory context and optionally zero it. ***********************************************************************************************************************************/ diff --git a/src/common/memContext.h b/src/common/memContext.h index b4113fc08..c15a3688d 100644 --- a/src/common/memContext.h +++ b/src/common/memContext.h @@ -66,6 +66,7 @@ Use the MEM_CONTEXT*() macros when possible rather than implement error-handling MemContext *memContextNew(const char *name); void memContextMove(MemContext *this, MemContext *parentNew); void memContextCallback(MemContext *this, void (*callbackFunction)(void *), void *callbackArgument); +void memContextCallbackClear(MemContext *this); MemContext *memContextSwitch(MemContext *this); void memContextFree(MemContext *this); diff --git a/src/common/regExp.c b/src/common/regExp.c index c8add9caf..849618e96 100644 --- a/src/common/regExp.c +++ b/src/common/regExp.c @@ -107,6 +107,8 @@ regExpFree(RegExp *this) if (this != NULL) { regfree(&this->regExp); + + memContextCallbackClear(this->memContext); memContextFree(this->memContext); } diff --git a/src/compress/gzipCompress.c b/src/compress/gzipCompress.c index 77197753a..e68ec2ae4 100644 --- a/src/compress/gzipCompress.c +++ b/src/compress/gzipCompress.c @@ -184,7 +184,7 @@ gzipCompressToLog(const GzipCompress *this) { return strNewFmt( "{inputSame: %s, done: %s, flushing: %s, availIn: %u}", cvtBoolToConstZ(this->inputSame), cvtBoolToConstZ(this->done), - cvtBoolToConstZ(this->done), this->stream != NULL ? this->stream->avail_in : 0); + cvtBoolToConstZ(this->done), this->stream->avail_in); } /*********************************************************************************************************************************** @@ -199,12 +199,10 @@ gzipCompressFree(GzipCompress *this) if (this != NULL) { - if (this->stream != NULL) - { - deflateEnd(this->stream); - this->stream = NULL; - } + deflateEnd(this->stream); + this->stream = NULL; + memContextCallbackClear(this->memContext); memContextFree(this->memContext); } diff --git a/src/compress/gzipDecompress.c b/src/compress/gzipDecompress.c index 1a067da4e..a917bfd53 100644 --- a/src/compress/gzipDecompress.c +++ b/src/compress/gzipDecompress.c @@ -159,7 +159,7 @@ gzipDecompressToLog(const GzipDecompress *this) { return strNewFmt( "{inputSame: %s, done: %s, availIn: %u}", cvtBoolToConstZ(this->inputSame), cvtBoolToConstZ(this->done), - this->stream != NULL ? this->stream->avail_in : 0); + this->stream->avail_in); } /*********************************************************************************************************************************** @@ -174,12 +174,9 @@ gzipDecompressFree(GzipDecompress *this) if (this != NULL) { - if (this->stream != NULL) - { - inflateEnd(this->stream); - this->stream = NULL; - } + inflateEnd(this->stream); + memContextCallbackClear(this->memContext); memContextFree(this->memContext); } diff --git a/src/crypto/cipherBlock.c b/src/crypto/cipherBlock.c index ac10937ec..5144b155c 100644 --- a/src/crypto/cipherBlock.c +++ b/src/crypto/cipherBlock.c @@ -291,6 +291,7 @@ cipherBlockFree(CipherBlock *this) EVP_CIPHER_CTX_cleanup(this->cipherContext); // Free mem context + memContextCallbackClear(this->memContext); memContextFree(this->memContext); FUNCTION_DEBUG_RESULT_VOID(); diff --git a/src/crypto/hash.c b/src/crypto/hash.c index 0d2b6cad1..9c1e0c574 100644 --- a/src/crypto/hash.c +++ b/src/crypto/hash.c @@ -219,12 +219,9 @@ cryptoHashFree(CryptoHash *this) if (this != NULL) { - if (this->hashContext != NULL) - { - EVP_MD_CTX_destroy(this->hashContext); - this->hashContext = NULL; - } + EVP_MD_CTX_destroy(this->hashContext); + memContextCallbackClear(this->memContext); memContextFree(this->memContext); } @@ -295,7 +292,6 @@ cryptoHashOneStr(const String *type, String *message) FUNCTION_TEST_RESULT(BUFFER, cryptoHashOneC(type, (const unsigned char *)strPtr(message), strSize(message))); } - /*********************************************************************************************************************************** Get hmac for one message/key ***********************************************************************************************************************************/ diff --git a/src/storage/driver/posix/fileRead.c b/src/storage/driver/posix/fileRead.c index 6cc2ae01c..0b33d3bb5 100644 --- a/src/storage/driver/posix/fileRead.c +++ b/src/storage/driver/posix/fileRead.c @@ -250,6 +250,8 @@ storageDriverPosixFileReadFree(StorageDriverPosixFileRead *this) if (this != NULL) { storageDriverPosixFileReadClose(this); + + memContextCallbackClear(this->memContext); memContextFree(this->memContext); } diff --git a/src/storage/driver/posix/fileWrite.c b/src/storage/driver/posix/fileWrite.c index 895486fb5..92ae554de 100644 --- a/src/storage/driver/posix/fileWrite.c +++ b/src/storage/driver/posix/fileWrite.c @@ -354,6 +354,8 @@ storageDriverPosixFileWriteFree(StorageDriverPosixFileWrite *this) if (this != NULL) { storageDriverPosixFileWriteClose(this); + + memContextCallbackClear(this->memContext); memContextFree(this->memContext); } diff --git a/test/src/module/common/memContextTest.c b/test/src/module/common/memContextTest.c index baa2a3666..e7bbf6bd5 100644 --- a/test/src/module/common/memContextTest.c +++ b/test/src/module/common/memContextTest.c @@ -232,6 +232,10 @@ testRun(void) memContextCallback(memContext, (MemContextCallback)testFree, memContext), AssertError, "callback is already set for context 'test-callback'"); + // Clear and reset it + memContextCallbackClear(memContext); + memContextCallback(memContext, (MemContextCallback)testFree, memContext); + memContextFree(memContext); TEST_RESULT_PTR(memContextCallbackArgument, memContext, "callback argument is context"); } diff --git a/test/src/module/compress/gzipTest.c b/test/src/module/compress/gzipTest.c index b769480d2..1b99f0dd9 100644 --- a/test/src/module/compress/gzipTest.c +++ b/test/src/module/compress/gzipTest.c @@ -170,8 +170,6 @@ testRun(void) decompress->inputSame = true; decompress->done = true; - inflateEnd(decompress->stream); - decompress->stream = NULL; TEST_RESULT_STR(strPtr(gzipDecompressToLog(decompress)), "{inputSame: true, done: true, availIn: 0}", "format object"); }