Обсуждение: [MASSMAIL]Confusing #if nesting in hmac_openssl.c

I noticed that buildfarm member batfish has been complaining like
this for awhile:

hmac_openssl.c:90:1: warning: unused function 'ResourceOwnerRememberHMAC' [-Wunused-function]
hmac_openssl.c:95:1: warning: unused function 'ResourceOwnerForgetHMAC' [-Wunused-function]

Looking at the code, this is all from commit e6bdfd970, and apparently
batfish is our only animal that doesn't HAVE_HMAC_CTX_NEW.  I tried to
understand the #if nesting and soon got very confused.  I don't think
it is helpful to put the resource owner manipulations inside #ifdef
HAVE_HMAC_CTX_NEW and HAVE_HMAC_CTX_FREE --- probably, it would never
be the case that only one of those is defined, but it just seems
messy.  What do you think of rearranging it as attached?

            regards, tom lane

diff --git a/src/common/hmac_openssl.c b/src/common/hmac_openssl.c
index d10f7e5af7..4a62c8832f 100644
--- a/src/common/hmac_openssl.c
+++ b/src/common/hmac_openssl.c
@@ -41,6 +41,7 @@
  */
 #ifndef FRONTEND
 #ifdef HAVE_HMAC_CTX_NEW
+#define USE_RESOWNER_FOR_HMAC
 #define ALLOC(size) MemoryContextAlloc(TopMemoryContext, size)
 #else
 #define ALLOC(size) palloc(size)
@@ -67,13 +68,13 @@ struct pg_hmac_ctx
     pg_hmac_errno error;
     const char *errreason;
 
-#ifndef FRONTEND
+#ifdef USE_RESOWNER_FOR_HMAC
     ResourceOwner resowner;
 #endif
 };
 
 /* ResourceOwner callbacks to hold HMAC contexts */
-#ifndef FRONTEND
+#ifdef USE_RESOWNER_FOR_HMAC
 static void ResOwnerReleaseHMAC(Datum res);
 
 static const ResourceOwnerDesc hmac_resowner_desc =
@@ -138,10 +139,12 @@ pg_hmac_create(pg_cryptohash_type type)
      * previous runs.
      */
     ERR_clear_error();
-#ifdef HAVE_HMAC_CTX_NEW
-#ifndef FRONTEND
+
+#ifdef USE_RESOWNER_FOR_HMAC
     ResourceOwnerEnlarge(CurrentResourceOwner);
 #endif
+
+#ifdef HAVE_HMAC_CTX_NEW
     ctx->hmacctx = HMAC_CTX_new();
 #else
     ctx->hmacctx = ALLOC(sizeof(HMAC_CTX));
@@ -159,14 +162,14 @@ pg_hmac_create(pg_cryptohash_type type)
         return NULL;
     }
 
-#ifdef HAVE_HMAC_CTX_NEW
-#ifndef FRONTEND
+#ifndef HAVE_HMAC_CTX_NEW
+    memset(ctx->hmacctx, 0, sizeof(HMAC_CTX));
+#endif                            /* HAVE_HMAC_CTX_NEW */
+
+#ifdef USE_RESOWNER_FOR_HMAC
     ctx->resowner = CurrentResourceOwner;
     ResourceOwnerRememberHMAC(CurrentResourceOwner, ctx);
 #endif
-#else
-    memset(ctx->hmacctx, 0, sizeof(HMAC_CTX));
-#endif                            /* HAVE_HMAC_CTX_NEW */
 
     return ctx;
 }
@@ -327,15 +330,16 @@ pg_hmac_free(pg_hmac_ctx *ctx)
 
 #ifdef HAVE_HMAC_CTX_FREE
     HMAC_CTX_free(ctx->hmacctx);
-#ifndef FRONTEND
-    if (ctx->resowner)
-        ResourceOwnerForgetHMAC(ctx->resowner, ctx);
-#endif
 #else
     explicit_bzero(ctx->hmacctx, sizeof(HMAC_CTX));
     FREE(ctx->hmacctx);
 #endif
 
+#ifdef USE_RESOWNER_FOR_HMAC
+    if (ctx->resowner)
+        ResourceOwnerForgetHMAC(ctx->resowner, ctx);
+#endif
+
     explicit_bzero(ctx, sizeof(pg_hmac_ctx));
     FREE(ctx);
 }
@@ -375,7 +379,7 @@ pg_hmac_error(pg_hmac_ctx *ctx)
 
 /* ResourceOwner callbacks */
 
-#ifndef FRONTEND
+#ifdef USE_RESOWNER_FOR_HMAC
 static void
 ResOwnerReleaseHMAC(Datum res)
 {

> On 2 Apr 2024, at 02:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> hmac_openssl.c:90:1: warning: unused function 'ResourceOwnerRememberHMAC' [-Wunused-function]
> hmac_openssl.c:95:1: warning: unused function 'ResourceOwnerForgetHMAC' [-Wunused-function]
>
> Looking at the code, this is all from commit e6bdfd970, and apparently
> batfish is our only animal that doesn't HAVE_HMAC_CTX_NEW.

Thanks for looking at this, it's been on my TODO for some time.  It's a warning
which only shows up when building against 1.0.2, the functions are present in
1.1.0 and onwards (while deprecated in 3.0).

> I don't think
> it is helpful to put the resource owner manipulations inside #ifdef
> HAVE_HMAC_CTX_NEW and HAVE_HMAC_CTX_FREE --- probably, it would never
> be the case that only one of those is defined,

Correct, no version of OpenSSL has only one of them defined.

> What do you think of rearranging it as attached?

+1 on this patch, it makes the #ifdef soup more readable.  We could go even
further and remove the HAVE_HMAC defines completely with USE_RESOWNER_FOR_HMAC
being set by autoconf/meson?  I've attached an untested sketch diff to
illustrate.

A related tangent.  If we assembled the data to calculate on ourselves rather
than rely on OpenSSL to do it with subsequent _update calls we could instead
use the simpler HMAC() API from OpenSSL.  That would remove the need for the
HMAC_CTX and resource owner tracking entirely and just have our pg_hmac_ctx.
Thats clearly not for this patch though, just thinking out loud that we set up
OpenSSL infrastructure that we don't really use.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
> On 2 Apr 2024, at 02:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think
>> it is helpful to put the resource owner manipulations inside #ifdef
>> HAVE_HMAC_CTX_NEW and HAVE_HMAC_CTX_FREE --- ...
>> What do you think of rearranging it as attached?

> +1 on this patch, it makes the #ifdef soup more readable.

Thanks for looking at it.

> We could go even
> further and remove the HAVE_HMAC defines completely with USE_RESOWNER_FOR_HMAC
> being set by autoconf/meson?  I've attached an untested sketch diff to
> illustrate.

I'm inclined to think that won't work, because we need the HAVE_
macros separately to compile correct frontend code.

> A related tangent.  If we assembled the data to calculate on ourselves rather
> than rely on OpenSSL to do it with subsequent _update calls we could instead
> use the simpler HMAC() API from OpenSSL.  That would remove the need for the
> HMAC_CTX and resource owner tracking entirely and just have our pg_hmac_ctx.
> Thats clearly not for this patch though, just thinking out loud that we set up
> OpenSSL infrastructure that we don't really use.

Simplifying like that could be good, but I'm not volunteering.
For the moment I'd just like to silence the buildfarm warning,
so I'll go ahead with what I have.

            regards, tom lane

> On 2 Apr 2024, at 15:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I'll go ahead with what I have.

+1

--
Daniel Gustafsson

On Tue, Apr 02, 2024 at 03:56:13PM +0200, Daniel Gustafsson wrote:
> > On 2 Apr 2024, at 15:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > I'll go ahead with what I have.
>
> +1

+#ifdef USE_RESOWNER_FOR_HMAC

Why not, that's cleaner.  Thanks for the commit.  The interactions
between this code and b8bff07da are interesting.
--
Michael