On 18/12/2020 11:35, Heikki Linnakangas wrote:
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.
Something like this. Passes regression tests, but otherwise untested.
- Heikki
diff --git a/src/common/cryptohash.c b/src/common/cryptohash.c
index cf4588bad72..d3fa32888f0 100644
--- a/src/common/cryptohash.c
+++ b/src/common/cryptohash.c
@@ -39,6 +39,20 @@
#define FREE(ptr) free(ptr)
#endif
+struct pg_cryptohash_ctx
+{
+ pg_cryptohash_type type;
+
+ union
+ {
+ pg_md5_ctx md5;
+ pg_sha224_ctx sha224;
+ pg_sha256_ctx sha256;
+ pg_sha384_ctx sha384;
+ pg_sha512_ctx sha512;
+ } data;
+};
+
/*
* pg_cryptohash_create
*
@@ -50,38 +64,15 @@ pg_cryptohash_create(pg_cryptohash_type type)
{
pg_cryptohash_ctx *ctx;
+ /*
+ * FIXME: this always allocates enough space for the largest hash.
+ * A smaller allocation would be enough for md5, sha224 and sha256.
+ */
ctx = ALLOC(sizeof(pg_cryptohash_ctx));
if (ctx == NULL)
return NULL;
-
ctx->type = type;
- switch (type)
- {
- case PG_MD5:
- ctx->data = ALLOC(sizeof(pg_md5_ctx));
- break;
- case PG_SHA224:
- ctx->data = ALLOC(sizeof(pg_sha224_ctx));
- break;
- case PG_SHA256:
- ctx->data = ALLOC(sizeof(pg_sha256_ctx));
- break;
- case PG_SHA384:
- ctx->data = ALLOC(sizeof(pg_sha384_ctx));
- break;
- case PG_SHA512:
- ctx->data = ALLOC(sizeof(pg_sha512_ctx));
- break;
- }
-
- if (ctx->data == NULL)
- {
- explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
- FREE(ctx);
- return NULL;
- }
-
return ctx;
}
@@ -100,19 +91,19 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
switch (ctx->type)
{
case PG_MD5:
- pg_md5_init((pg_md5_ctx *) ctx->data);
+ pg_md5_init(&ctx->data.md5);
break;
case PG_SHA224:
- pg_sha224_init((pg_sha224_ctx *) ctx->data);
+ pg_sha224_init(&ctx->data.sha224);
break;
case PG_SHA256:
- pg_sha256_init((pg_sha256_ctx *) ctx->data);
+ pg_sha256_init(&ctx->data.sha256);
break;
case PG_SHA384:
- pg_sha384_init((pg_sha384_ctx *) ctx->data);
+ pg_sha384_init(&ctx->data.sha384);
break;
case PG_SHA512:
- pg_sha512_init((pg_sha512_ctx *) ctx->data);
+ pg_sha512_init(&ctx->data.sha512);
break;
}
@@ -134,19 +125,19 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
switch (ctx->type)
{
case PG_MD5:
- pg_md5_update((pg_md5_ctx *) ctx->data, data, len);
+ pg_md5_update(&ctx->data.md5, data, len);
break;
case PG_SHA224:
- pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
+ pg_sha224_update(&ctx->data.sha224, data, len);
break;
case PG_SHA256:
- pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
+ pg_sha256_update(&ctx->data.sha256, data, len);
break;
case PG_SHA384:
- pg_sha384_update((pg_sha384_ctx *) ctx->data, data, len);
+ pg_sha384_update(&ctx->data.sha384, data, len);
break;
case PG_SHA512:
- pg_sha512_update((pg_sha512_ctx *) ctx->data, data, len);
+ pg_sha512_update(&ctx->data.sha512, data, len);
break;
}
@@ -168,19 +159,19 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
switch (ctx->type)
{
case PG_MD5:
- pg_md5_final((pg_md5_ctx *) ctx->data, dest);
+ pg_md5_final(&ctx->data.md5, dest);
break;
case PG_SHA224:
- pg_sha224_final((pg_sha224_ctx *) ctx->data, dest);
+ pg_sha224_final(&ctx->data.sha224, dest);
break;
case PG_SHA256:
- pg_sha256_final((pg_sha256_ctx *) ctx->data, dest);
+ pg_sha256_final(&ctx->data.sha256, dest);
break;
case PG_SHA384:
- pg_sha384_final((pg_sha384_ctx *) ctx->data, dest);
+ pg_sha384_final(&ctx->data.sha384, dest);
break;
case PG_SHA512:
- pg_sha512_final((pg_sha512_ctx *) ctx->data, dest);
+ pg_sha512_final(&ctx->data.sha512, dest);
break;
}
@@ -198,26 +189,6 @@ pg_cryptohash_free(pg_cryptohash_ctx *ctx)
if (ctx == NULL)
return;
- switch (ctx->type)
- {
- case PG_MD5:
- explicit_bzero(ctx->data, sizeof(pg_md5_ctx));
- break;
- case PG_SHA224:
- explicit_bzero(ctx->data, sizeof(pg_sha224_ctx));
- break;
- case PG_SHA256:
- explicit_bzero(ctx->data, sizeof(pg_sha256_ctx));
- break;
- case PG_SHA384:
- explicit_bzero(ctx->data, sizeof(pg_sha384_ctx));
- break;
- case PG_SHA512:
- explicit_bzero(ctx->data, sizeof(pg_sha512_ctx));
- break;
- }
-
- FREE(ctx->data);
explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
FREE(ctx);
}
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 118651c4153..24b9636d1ca 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)
@@ -43,19 +43,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 +69,41 @@ 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;
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));
#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,33 +120,30 @@ 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;
-
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;
}
- /* OpenSSL internals return 1 on success, 0 on failure */
+ /* openssl internals return 1 on success, 0 on failure */
if (status <= 0)
return -1;
return 0;
@@ -161,21 +152,19 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
/*
* pg_cryptohash_update
*
- * Update a hash context. Returns 0 on success, and -1 on failure.
+ * update a hash context. returns 0 on success, and -1 on failure.
*/
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;
- 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 */
+ /* openssl internals return 1 on success, 0 on failure */
if (status <= 0)
return -1;
return 0;
@@ -184,19 +173,17 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
/*
* pg_cryptohash_final
*
- * Finalize a hash context. Returns 0 on success, and -1 on failure.
+ * finalize a hash context. returns 0 on success, and -1 on failure.
*/
int
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
{
int status = 0;
- pg_cryptohash_state *state;
if (ctx == NULL)
return 0;
- 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)
@@ -207,26 +194,21 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
/*
* pg_cryptohash_free
*
- * Free a hash context.
+ * free a hash context.
*/
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);
}
diff --git a/src/include/common/cryptohash.h b/src/include/common/cryptohash.h
index 6ead1cb8e5b..69e69532d44 100644
--- a/src/include/common/cryptohash.h
+++ b/src/include/common/cryptohash.h
@@ -25,12 +25,8 @@ typedef enum
PG_SHA512
} pg_cryptohash_type;
-typedef struct pg_cryptohash_ctx
-{
- pg_cryptohash_type type;
- /* private area used by each hash implementation */
- void *data;
-} pg_cryptohash_ctx;
+/* opaque context, private to the cryptohash implementation */
+typedef struct pg_cryptohash_ctx pg_cryptohash_ctx;
extern pg_cryptohash_ctx *pg_cryptohash_create(pg_cryptohash_type type);
extern int pg_cryptohash_init(pg_cryptohash_ctx *ctx);
