On 18/12/2020 09:35, Michael Paquier wrote:
Hi all,
As of the work done in 87ae9691, I have played with error injections
in the code paths using this code, but forgot to count for cases where
cascading resowner cleanups are involved. Like other resources (JIT,
DSM, etc.), this requires an allocation in TopMemoryContext to make
sure that nothing gets forgotten or cleaned up on the way until the
resowner that did the cryptohash allocation is handled.
Attached is a small extension I have played with by doing some error
injections, and a patch. If there are no objections, I would like to
commit this fix.
pg_cryptohash_create() is now susceptible to leaking memory in
TopMemoryContext, if the allocations fail. I think the attached should
fix it (but I haven't tested it at all).
BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we
need two structs? They're both allocated and controlled by the
cryptohash implementation. It would seem simpler to have just one.
- Heikki
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 118651c4153..bb303dd6c8d 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -35,7 +35,7 @@
* use malloc to be able to return a failure status back to the caller.
*/
#ifndef FRONTEND
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAllocExtended(TopMemoryContext, size, MCXT_ALLOC_NO_OOM)
#define FREE(ptr) pfree(ptr)
#else
#define ALLOC(size) malloc(size)
@@ -69,6 +69,10 @@ pg_cryptohash_create(pg_cryptohash_type type)
pg_cryptohash_ctx *ctx;
pg_cryptohash_state *state;
+#ifndef FRONTEND
+ ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
+#endif
+
ctx = ALLOC(sizeof(pg_cryptohash_ctx));
if (ctx == NULL)
return NULL;
@@ -84,10 +88,6 @@ pg_cryptohash_create(pg_cryptohash_type type)
ctx->data = state;
ctx->type = type;
-#ifndef FRONTEND
- ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
-#endif
-
/*
* Initialization takes care of assigning the correct type for OpenSSL.
*/
