On Tue, Dec 01, 2020 at 10:10:45AM +0100, Daniel Gustafsson wrote: > That sounds like it would work. Since the cryptohash implementation owns and > controls the void *data contents it can store whatever it needs to properly > free it.
That seems to work properly. I have injected some elog(ERROR) with the attached in some of the SQL functions from cryptohashes.c and the cleanup happens with the correct resowner references. It felt a bit strange to use directly the term EVP in resowner.c once I did this switch, so this is renamed to "CryptoHash" instead. > Reading through the v6 patch I see nothing sticking out and all review > comments > addressed, +1 on applying that one and then we'll take if from there with the > remaining ones in the patchset. Thanks. 0001 has been applied and the buildfarm does not complain, so it looks like we are good (I'll take care of any issues, like the one Fujii-san has just reported). Attached are new patches for 0002, the EVP switch. One thing I noticed is that we need to free the backup manifest a bit earlier once we begin to use resource owner in basebackup.c as there is a specific step that may do a double-free. This would not happen when not using OpenSSL or on HEAD. It would be easy to separate the resowner and cryptohash portions of the patch here, but both are tightly linked, so I'd prefer to keep them together. Another thing to note is that this makes 0003 for pgcrypto meaningless, because this would track down only states created by cryptohash_openssl.c, and not directly EVP_MD_CTX. Consistency in the backend code is for the best, and considering that pgcrypto has similar linked lists for ciphers it feels a bit weird to switch only half of it to use something in resowner.c. So, I am not sure if we need to do anything here, and I am discarding this part. -- Michael
From d2c9f2a9c9a36d80a014d588334e21a709a1c27d Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 2 Dec 2020 11:51:43 +0900
Subject: [PATCH v7] Switch cryptohash_openssl.c to use EVP
Postgres is two decades late for this switch.
---
src/include/utils/resowner_private.h | 7 ++
src/backend/replication/basebackup.c | 8 +-
src/backend/utils/resowner/resowner.c | 62 ++++++++++++
src/common/cryptohash_openssl.c | 132 ++++++++++++++++----------
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 158 insertions(+), 52 deletions(-)
diff --git a/src/include/utils/resowner_private.h b/src/include/utils/resowner_private.h
index a781a7a2aa..c373788bc1 100644
--- a/src/include/utils/resowner_private.h
+++ b/src/include/utils/resowner_private.h
@@ -95,4 +95,11 @@ extern void ResourceOwnerRememberJIT(ResourceOwner owner,
extern void ResourceOwnerForgetJIT(ResourceOwner owner,
Datum handle);
+/* support for cryptohash context management */
+extern void ResourceOwnerEnlargeCryptoHash(ResourceOwner owner);
+extern void ResourceOwnerRememberCryptoHash(ResourceOwner owner,
+ Datum handle);
+extern void ResourceOwnerForgetCryptoHash(ResourceOwner owner,
+ Datum handle);
+
#endif /* RESOWNER_PRIVATE_H */
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 22be7ca9d5..79b458c185 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -729,11 +729,17 @@ perform_base_backup(basebackup_options *opt)
errmsg("checksum verification failure during base backup")));
}
+ /*
+ * Make sure to free the manifest before the resource owners as
+ * manifests use cryptohash contexts that may depend on resource
+ * owners (like OpenSSL).
+ */
+ FreeBackupManifest(&manifest);
+
/* clean up the resource owner we created */
WalSndResourceCleanup(true);
pgstat_progress_end_command();
- FreeBackupManifest(&manifest);
}
/*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index 8bc2c4e9ea..546ad8d1c5 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -20,6 +20,7 @@
*/
#include "postgres.h"
+#include "common/cryptohash.h"
#include "common/hashfn.h"
#include "jit/jit.h"
#include "storage/bufmgr.h"
@@ -128,6 +129,7 @@ typedef struct ResourceOwnerData
ResourceArray filearr; /* open temporary files */
ResourceArray dsmarr; /* dynamic shmem segments */
ResourceArray jitarr; /* JIT contexts */
+ ResourceArray cryptohasharr; /* cryptohash contexts */
/* We can remember up to MAX_RESOWNER_LOCKS references to local locks. */
int nlocks; /* number of owned locks */
@@ -175,6 +177,7 @@ static void PrintTupleDescLeakWarning(TupleDesc tupdesc);
static void PrintSnapshotLeakWarning(Snapshot snapshot);
static void PrintFileLeakWarning(File file);
static void PrintDSMLeakWarning(dsm_segment *seg);
+static void PrintCryptoHashLeakWarning(Datum handle);
/*****************************************************************************
@@ -444,6 +447,7 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name)
ResourceArrayInit(&(owner->filearr), FileGetDatum(-1));
ResourceArrayInit(&(owner->dsmarr), PointerGetDatum(NULL));
ResourceArrayInit(&(owner->jitarr), PointerGetDatum(NULL));
+ ResourceArrayInit(&(owner->cryptohasharr), PointerGetDatum(NULL));
return owner;
}
@@ -553,6 +557,17 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
jit_release_context(context);
}
+
+ /* Ditto for cryptohash contexts */
+ while (ResourceArrayGetAny(&(owner->cryptohasharr), &foundres))
+ {
+ pg_cryptohash_ctx *context =
+ (pg_cryptohash_ctx *) PointerGetDatum(foundres);
+
+ if (isCommit)
+ PrintCryptoHashLeakWarning(foundres);
+ pg_cryptohash_free(context);
+ }
}
else if (phase == RESOURCE_RELEASE_LOCKS)
{
@@ -725,6 +740,7 @@ ResourceOwnerDelete(ResourceOwner owner)
Assert(owner->filearr.nitems == 0);
Assert(owner->dsmarr.nitems == 0);
Assert(owner->jitarr.nitems == 0);
+ Assert(owner->cryptohasharr.nitems == 0);
Assert(owner->nlocks == 0 || owner->nlocks == MAX_RESOWNER_LOCKS + 1);
/*
@@ -752,6 +768,7 @@ ResourceOwnerDelete(ResourceOwner owner)
ResourceArrayFree(&(owner->filearr));
ResourceArrayFree(&(owner->dsmarr));
ResourceArrayFree(&(owner->jitarr));
+ ResourceArrayFree(&(owner->cryptohasharr));
pfree(owner);
}
@@ -1370,3 +1387,48 @@ ResourceOwnerForgetJIT(ResourceOwner owner, Datum handle)
elog(ERROR, "JIT context %p is not owned by resource owner %s",
DatumGetPointer(handle), owner->name);
}
+
+/*
+ * Make sure there is room for at least one more entry in a ResourceOwner's
+ * cryptohash context reference array.
+ *
+ * This is separate from actually inserting an entry because if we run out of
+ * memory, it's critical to do so *before* acquiring the resource.
+ */
+void
+ResourceOwnerEnlargeCryptoHash(ResourceOwner owner)
+{
+ ResourceArrayEnlarge(&(owner->cryptohasharr));
+}
+
+/*
+ * Remember that a cryptohash context is owned by a ResourceOwner
+ *
+ * Caller must have previously done ResourceOwnerEnlargeCryptoHash()
+ */
+void
+ResourceOwnerRememberCryptoHash(ResourceOwner owner, Datum handle)
+{
+ ResourceArrayAdd(&(owner->cryptohasharr), handle);
+}
+
+/*
+ * Forget that a cryptohash context is owned by a ResourceOwner
+ */
+void
+ResourceOwnerForgetCryptoHash(ResourceOwner owner, Datum handle)
+{
+ if (!ResourceArrayRemove(&(owner->cryptohasharr), handle))
+ elog(ERROR, "cryptohash context %p is not owned by resource owner %s",
+ DatumGetPointer(handle), owner->name);
+}
+
+/*
+ * Debugging subroutine
+ */
+static void
+PrintCryptoHashLeakWarning(Datum handle)
+{
+ elog(WARNING, "cryptohash context reference leak: context %p still referenced",
+ DatumGetPointer(handle));
+}
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 8e2c69b48b..d0f58b81be 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -21,9 +21,14 @@
#include "postgres_fe.h"
#endif
-#include <openssl/sha.h>
+#include <openssl/evp.h>
#include "common/cryptohash.h"
+#ifndef FRONTEND
+#include "utils/memutils.h"
+#include "utils/resowner.h"
+#include "utils/resowner_private.h"
+#endif
/*
* In backend, use palloc/pfree to ease the error handling. In frontend,
@@ -37,6 +42,21 @@
#define FREE(ptr) free(ptr)
#endif
+/*
+ * Internal structure for pg_cryptohash_ctx->data.
+ *
+ * This tracks the resource owner associated to each EVP context data
+ * for the backend.
+ */
+typedef struct pg_cryptohash_state
+{
+ EVP_MD_CTX *evpctx;
+
+#ifndef FRONTEND
+ ResourceOwner resowner;
+#endif
+} pg_cryptohash_state;
+
/*
* pg_cryptohash_create
*
@@ -47,32 +67,51 @@ 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;
- ctx->type = type;
-
- switch (type)
- {
- case PG_SHA224:
- case PG_SHA256:
- ctx->data = ALLOC(sizeof(SHA256_CTX));
- break;
- case PG_SHA384:
- case PG_SHA512:
- ctx->data = ALLOC(sizeof(SHA512_CTX));
- break;
- }
-
- if (ctx->data == 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();
+
+ if (state->evpctx == NULL)
+ {
+ explicit_bzero(state, sizeof(pg_cryptohash_state));
+ explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
+#ifndef FRONTEND
+ elog(ERROR, "out of memory");
+#else
+ FREE(state);
+ FREE(ctx);
+ return NULL;
+#endif
+ }
+
+#ifndef FRONTEND
+ state->resowner = CurrentResourceOwner;
+ ResourceOwnerRememberCryptoHash(CurrentResourceOwner,
+ PointerGetDatum(ctx));
+#endif
+
return ctx;
}
@@ -85,23 +124,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;
+
switch (ctx->type)
{
case PG_SHA224:
- status = SHA224_Init((SHA256_CTX *) ctx->data);
+ status = EVP_DigestInit_ex(state->evpctx, EVP_sha224(), NULL);
break;
case PG_SHA256:
- status = SHA256_Init((SHA256_CTX *) ctx->data);
+ status = EVP_DigestInit_ex(state->evpctx, EVP_sha256(), NULL);
break;
case PG_SHA384:
- status = SHA384_Init((SHA512_CTX *) ctx->data);
+ status = EVP_DigestInit_ex(state->evpctx, EVP_sha384(), NULL);
break;
case PG_SHA512:
- status = SHA512_Init((SHA512_CTX *) ctx->data);
+ status = EVP_DigestInit_ex(state->evpctx, EVP_sha512(), NULL);
break;
}
@@ -120,25 +162,13 @@ int
pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
{
int status;
+ pg_cryptohash_state *state;
if (ctx == NULL)
return 0;
- switch (ctx->type)
- {
- case PG_SHA224:
- status = SHA224_Update((SHA256_CTX *) ctx->data, data, len);
- break;
- case PG_SHA256:
- status = SHA256_Update((SHA256_CTX *) ctx->data, data, len);
- break;
- case PG_SHA384:
- status = SHA384_Update((SHA512_CTX *) ctx->data, data, len);
- break;
- case PG_SHA512:
- status = SHA512_Update((SHA512_CTX *) ctx->data, data, len);
- break;
- }
+ state = (pg_cryptohash_state *) ctx->data;
+ status = EVP_DigestUpdate(state->evpctx, data, len);
/* OpenSSL internals return 1 on success, 0 on failure */
if (status <= 0)
@@ -155,25 +185,13 @@ int
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
{
int status;
+ pg_cryptohash_state *state;
if (ctx == NULL)
return 0;
- switch (ctx->type)
- {
- case PG_SHA224:
- status = SHA224_Final(dest, (SHA256_CTX *) ctx->data);
- break;
- case PG_SHA256:
- status = SHA256_Final(dest, (SHA256_CTX *) ctx->data);
- break;
- case PG_SHA384:
- status = SHA384_Final(dest, (SHA512_CTX *) ctx->data);
- break;
- case PG_SHA512:
- status = SHA512_Final(dest, (SHA512_CTX *) ctx->data);
- break;
- }
+ state = (pg_cryptohash_state *) ctx->data;
+ status = EVP_DigestFinal_ex(state->evpctx, dest, 0);
/* OpenSSL internals return 1 on success, 0 on failure */
if (status <= 0)
@@ -189,9 +207,21 @@ 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;
- FREE(ctx->data);
+
+ state = (pg_cryptohash_state *) ctx->data;
+ EVP_MD_CTX_destroy(state->evpctx);
+
+#ifndef FRONTEND
+ ResourceOwnerForgetCryptoHash(state->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 04464c2e76..cf63acbf6f 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3180,6 +3180,7 @@ pg_conv_map
pg_crc32
pg_crc32c
pg_cryptohash_ctx
+pg_cryptohash_state
pg_cryptohash_type
pg_ctype_cache
pg_enc
--
2.29.2
signature.asc
Description: PGP signature
