Confusing #if nesting in hmac_openssl.c

  • Jump to comment-1
    tgl@sss.pgh.pa.us2024-04-02T00:01:47+00:00
    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
    • Jump to comment-1
      daniel@yesql.se2024-04-02T12:18:22+00:00
      > 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
      • Jump to comment-1
        tgl@sss.pgh.pa.us2024-04-02T13:50:27+00:00
        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
        • Jump to comment-1
          daniel@yesql.se2024-04-02T13:56:21+00:00
          > 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
          • Jump to comment-1
            michael@paquier.xyz2024-04-03T06:18:30+00:00
            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