On Sat, Dec 19, 2020 at 03:13:50PM +0900, Michael Paquier wrote: > 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?
I have looked again at this thread with a fresher mind and I did not see a problem with the previous patch, except some indentation issues. So if there are no objections, I'd like to commit the attached. -- Michael
diff --git a/src/include/common/cryptohash.h b/src/include/common/cryptohash.h
index 4c3df8e5ae..3ecaf62113 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 1921a33b34..efedadd626 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 0fd6622576..551ec392b6 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);
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9cd047ba25..f3957bad6c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3196,7 +3196,6 @@ pg_conv_map
pg_crc32
pg_crc32c
pg_cryptohash_ctx
-pg_cryptohash_state
pg_cryptohash_type
pg_ctype_cache
pg_enc
signature.asc
Description: PGP signature
