On Fri, Dec 18, 2020 at 11:51:55AM +0200, Heikki Linnakangas wrote: > 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.
Interesting. I have looked at that with a fresh mind, thanks for the
idea. This reduces the number of allocations to one making the error
handling a no-brainer, at the cost of hiding the cryptohash type
directly to the caller. I originally thought that this would be
useful as I recall reading cases in the OpenSSL code doing checks on
hash type used, but perhaps that's just some over-engineered thoughts
from my side. I have found a couple of small-ish issues, please see
my comments below.
+ /*
+ * FIXME: this always allocates enough space for the largest hash.
+ * A smaller allocation would be enough for md5, sha224 and sha256.
+ */
I am not sure that this is worth complicating more, and we are not
talking about a lot of memory (sha512 requires 208 bytes, sha224/256
104 bytes, md5 96 bytes with a quick measurement). This makes free()
equally more simple. So just allocating the amount of memory based on
the max size in the union looks fine to me. I would add a memset(0)
after this allocation though.
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAllocExtended(TopMemoryContext, size,
MCXT_ALLOC_NO_OOM)
As the only allocation in TopMemoryContext is for the context, it
would be fine to not use MCXT_ALLOC_NO_OOM here, and fail so as
callers in the backend don't need to worry about create() returning
NULL.
- state->evpctx = EVP_MD_CTX_create();
+ ctx->evpctx = EVP_MD_CTX_create();
- if (state->evpctx == NULL)
+ if (ctx->evpctx == NULL)
{
If EVP_MD_CTX_create() fails, you would leak memory with the context
allocated in TopMemoryContext. So this requires a free of the context
before the elog(ERROR).
+ /*
+ * 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
Right. Good point.
- /* OpenSSL internals return 1 on success, 0 on failure */
+ /* openssl internals return 1 on success, 0 on failure */
It seems to me that this was not wanted.
At the same time, I have taken care of your comment from upthread to
return a failure if the caller passes NULL for the context, and
adjusted some comments. What do you think of the attached?
--
Michael
diff --git a/src/include/common/cryptohash.h b/src/include/common/cryptohash.h
index 6ead1cb8e5..817e07c9a2 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 each 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);
diff --git a/src/common/cryptohash.c b/src/common/cryptohash.c
index cf4588bad7..f385bd778f 100644
--- a/src/common/cryptohash.c
+++ b/src/common/cryptohash.c
@@ -39,6 +39,21 @@
#define FREE(ptr) free(ptr)
#endif
+/* Internal pg_cryptohash_ctx structure */
+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 +65,18 @@ pg_cryptohash_create(pg_cryptohash_type type)
{
pg_cryptohash_ctx *ctx;
+ /*
+ * Note that this always allocates enough space for the largest hash.
+ * A smaller allocation would be enough for md5, sha224 and sha256,
+ * but the small extra amount of memory does not make it worth
+ * complicating this code.
+ */
ctx = ALLOC(sizeof(pg_cryptohash_ctx));
if (ctx == NULL)
return NULL;
-
+ memset(ctx, 0, sizeof(pg_cryptohash_ctx));
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;
}
@@ -95,24 +90,24 @@ int
pg_cryptohash_init(pg_cryptohash_ctx *ctx)
{
if (ctx == NULL)
- return 0;
+ return -1;
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;
}
@@ -123,30 +118,31 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
* pg_cryptohash_update
*
* Update a hash context. Note that this implementation is designed
- * to never fail, so this always returns 0.
+ * to never fail, so this always returns 0 except if the caller has
+ * given a NULL context.
*/
int
pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
{
if (ctx == NULL)
- return 0;
+ return -1;
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;
}
@@ -157,30 +153,31 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
* pg_cryptohash_final
*
* Finalize a hash context. Note that this implementation is designed
- * to never fail, so this always returns 0.
+ * to never fail, so this always returns 0 except if the caller has
+ * given a NULL context.
*/
int
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
{
if (ctx == NULL)
- return 0;
+ return -1;
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 +195,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 118651c415..c5077664f5 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;
-
- 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;
+ /*
+ * 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;
+ memset(ctx, 0, sizeof(pg_cryptohash_ctx));
+ ctx->type = type;
+
/*
* 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);
}
signature.asc
Description: PGP signature
