On Wed, Dec 02, 2020 at 12:03:49PM +0900, Michael Paquier wrote: > 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.
Attached is a rebased version to take care of the conflicts introduced by 91624c2f. -- Michael
From a68819b3f843b4ce2883c35c008d5dd4fb47ee35 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 2 Dec 2020 11:51:43 +0900
Subject: [PATCH v8] 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 33f17cac33..3c88ae648e 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 = 0;
+ 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 = 0;
+ 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
