Fix allocation logic of cryptohash context data with OpenSSL

The allocation of the cryptohash context data when building with OpenSSL
was happening in the memory context of the caller of
pg_cryptohash_create(), which could lead to issues with resowner cleanup
if cascading resources are cleaned up on an error.  Like other
facilities using resowners, move the base allocation to TopMemoryContext
to ensure a correct cleanup on failure.

The resulting code gets simpler with this commit as the context data is
now hold by a unique opaque pointer, so as there is only one single
allocation done in TopMemoryContext.

After discussion, also change the cryptohash subroutines to return an
error if the caller provides NULL for the context data to ease error
detection on OOM.

Author: Heikki Linnakangas
Discussion: https://postgr.es/m/[email protected]

1 files changed, 35 insertions, 51 deletions

diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 0fd66225764..551ec392b60 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -31,11 +31,12 @@
 #endif
 
 /*
- * In backend, use palloc/pfree to ease the error handling.  In frontend,
- * use malloc to be able to return a failure status back to the caller.
+ * In the backend, use an allocation in TopMemoryContext to count for
+ * resowner cleanup handling.  In the frontend, use malloc to be able
+ * to return a failure status back to the caller.
  */
 #ifndef FRONTEND
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAlloc(TopMemoryContext, size)
 #define FREE(ptr) pfree(ptr)
 #else
 #define ALLOC(size) malloc(size)
@@ -43,19 +44,21 @@
 #endif
 
 /*
- * Internal structure for pg_cryptohash_ctx->data.
+ * Internal pg_cryptohash_ctx structure.
  *
  * This tracks the resource owner associated to each EVP context data
  * for the backend.
  */
-typedef struct pg_cryptohash_state
+struct pg_cryptohash_ctx
 {
+	pg_cryptohash_type type;
+
 	EVP_MD_CTX *evpctx;
 
 #ifndef FRONTEND
 	ResourceOwner resowner;
 #endif
-} pg_cryptohash_state;
+};
 
 /*
  * pg_cryptohash_create
@@ -67,49 +70,42 @@ pg_cryptohash_ctx *
 pg_cryptohash_create(pg_cryptohash_type type)
 {
 	pg_cryptohash_ctx *ctx;
-	pg_cryptohash_state *state;
+
+	/*
+	 * Make sure that the resource owner has space to remember this reference.
+	 * This can error out with "out of memory", so do this before any other
+	 * allocation to avoid leaking.
+	 */
+#ifndef FRONTEND
+	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
+#endif
 
 	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
 	if (ctx == NULL)
 		return NULL;
-
-	state = ALLOC(sizeof(pg_cryptohash_state));
-	if (state == NULL)
-	{
-		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-		FREE(ctx);
-		return NULL;
-	}
-
-	ctx->data = state;
+	memset(ctx, 0, sizeof(pg_cryptohash_ctx));
 	ctx->type = type;
 
-#ifndef FRONTEND
-	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
-#endif
-
 	/*
 	 * Initialization takes care of assigning the correct type for OpenSSL.
 	 */
-	state->evpctx = EVP_MD_CTX_create();
+	ctx->evpctx = EVP_MD_CTX_create();
 
-	if (state->evpctx == NULL)
+	if (ctx->evpctx == NULL)
 	{
-		explicit_bzero(state, sizeof(pg_cryptohash_state));
 		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
+		FREE(ctx);
 #ifndef FRONTEND
 		ereport(ERROR,
 				(errcode(ERRCODE_OUT_OF_MEMORY),
 				 errmsg("out of memory")));
 #else
-		FREE(state);
-		FREE(ctx);
 		return NULL;
 #endif
 	}
 
 #ifndef FRONTEND
-	state->resowner = CurrentResourceOwner;
+	ctx->resowner = CurrentResourceOwner;
 	ResourceOwnerRememberCryptoHash(CurrentResourceOwner,
 									PointerGetDatum(ctx));
 #endif
@@ -126,29 +122,26 @@ int
 pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
-
-	state = (pg_cryptohash_state *) ctx->data;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_md5(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_md5(), NULL);
 			break;
 		case PG_SHA224:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha224(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha224(), NULL);
 			break;
 		case PG_SHA256:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha256(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha256(), NULL);
 			break;
 		case PG_SHA384:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha384(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha384(), NULL);
 			break;
 		case PG_SHA512:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha512(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha512(), NULL);
 			break;
 	}
 
@@ -167,13 +160,11 @@ int
 pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestUpdate(state->evpctx, data, len);
+	status = EVP_DigestUpdate(ctx->evpctx, data, len);
 
 	/* OpenSSL internals return 1 on success, 0 on failure */
 	if (status <= 0)
@@ -190,13 +181,11 @@ int
 pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestFinal_ex(state->evpctx, dest, 0);
+	status = EVP_DigestFinal_ex(ctx->evpctx, dest, 0);
 
 	/* OpenSSL internals return 1 on success, 0 on failure */
 	if (status <= 0)
@@ -212,21 +201,16 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 void
 pg_cryptohash_free(pg_cryptohash_ctx *ctx)
 {
-	pg_cryptohash_state *state;
-
 	if (ctx == NULL)
 		return;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	EVP_MD_CTX_destroy(state->evpctx);
+	EVP_MD_CTX_destroy(ctx->evpctx);
 
 #ifndef FRONTEND
-	ResourceOwnerForgetCryptoHash(state->resowner,
+	ResourceOwnerForgetCryptoHash(ctx->resowner,
 								  PointerGetDatum(ctx));
 #endif
 
-	explicit_bzero(state, sizeof(pg_cryptohash_state));
 	explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-	FREE(state);
 	FREE(ctx);
 }