Обсуждение: __pg_log_level in anonynous enum should be initialized? (Was: pgsql: Change SHA2 implementation based on OpenSSL to use EVP digest ro)

Hi all,
(Moved the discussion to -hackers)

On Mon, Sep 28, 2020 at 03:48:12AM +0000, Michael Paquier wrote:
> Change SHA2 implementation based on OpenSSL to use EVP digest routines
>
> The use of low-level hash routines is not recommended by upstream
> OpenSSL since 2000, and pgcrypto already switched to EVP as of 5ff4a67.
> Note that this also fixes a failure with SCRAM authentication when using
> FIPS in OpenSSL, but as there have been few complaints about this
> problem and as this causes an ABI breakage, no backpatch is done.

It looks like this commit has broken prairiedog as follows:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2020-09-28%2005%3A45%3A55
ld: common symbols not allowed with MH_DYLIB output format with the
-multi_module option
../../../src/common/libpgcommon_shlib.a(logging_shlib.o) definition of
common ___pg_log_level (size 4)

Looking around, I have found a similar report from MacPorts and 10.4,
leading to the fact that it may be better to initialize
__pg_log_level:
https://trac.macports.org/ticket/19829

This has visibly not popped up until now because jsonapi.c,
controldata_utils.c and file_utils.c are not getting used by libpq,
though libpgcommon_shlib.a includes them.  And libpq uses
fe-auth-scram.c, that itself depends scram-common.c, and then depends
on the SHA2 routines.  Shouldn't __pg_log_level be initialized in
logging.c?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> It looks like this commit has broken prairiedog as follows:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2020-09-28%2005%3A45%3A55
> ld: common symbols not allowed with MH_DYLIB output format with the
> -multi_module option
> ../../../src/common/libpgcommon_shlib.a(logging_shlib.o) definition of
> common ___pg_log_level (size 4)

> Looking around, I have found a similar report from MacPorts and 10.4,
> leading to the fact that it may be better to initialize
> __pg_log_level:
> https://trac.macports.org/ticket/19829

I experimented and confirmed that explicitly initializing __pg_log_level
would suppress this build error on prairiedog.  However, I'm not sure
that doing so is a good idea, because it seems to me that this animal
has accidentally saved us from a serious design error.  IMO it is quite
unacceptable for libpq to contain either a forced write to stderr or a
forced exit(1).  It should be reporting the failure back to the calling
application, instead.  So the error handling in this patch needs to be
reconsidered.

Since prairiedog is not likely to be around forever, I propose that
we ought to enforce this going forward by arranging for common/logging.c
to not get built into the libpgcommon_shlib library variant.

            regards, tom lane

On Mon, Sep 28, 2020 at 12:49:29PM -0400, Tom Lane wrote:
> I experimented and confirmed that explicitly initializing __pg_log_level
> would suppress this build error on prairiedog.  However, I'm not sure
> that doing so is a good idea, because it seems to me that this animal
> has accidentally saved us from a serious design error.  IMO it is quite
> unacceptable for libpq to contain either a forced write to stderr or a
> forced exit(1).  It should be reporting the failure back to the calling
> application, instead.  So the error handling in this patch needs to be
> reconsidered.

Yeah, I have arrived at the same conclusion after a night of sleep and
after looking at the OpenSSL code for a couple of hours to see if it
would be possible to use a static state of EVP and not have to do all
this error handling.  But, we are going to need much more than that,
so I have reverted the patch:
- Any allocation done with EVP is going to need a callback for the
cleanup in the backend, because we basically have to do the allocation
directly in OpenSSL.  Not only for the higher EVP structure we get to
use in sha2_openssl.c, but also for some other parts at lower level,
like MD with EVP_MD_CTX_FLAG_NO_INIT.
- It will be better to have those routines return a status and just
let the caller handle the error.  This impacts the internals of SCRAM,
particularly for the secret and internal HMAC implementations.  And
this needs a change in the fallback implementation.
- We will most likely need to split the existing init and destroy
routines so as the initial allocation and final free happen in
different paths, even for the fallback implementation so as we have a
1:1 mapping with what OpenSSL does.

> Since prairiedog is not likely to be around forever, I propose that
> we ought to enforce this going forward by arranging for common/logging.c
> to not get built into the libpgcommon_shlib library variant.

Agreed.  This makes sense in the long term, so attached is a proposal
of patch.  I did not really see the point in complicating the MSVC
scripts for that though.  The new variable names look natural to me
like that, the new comment may need some tweaks.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Sep 28, 2020 at 12:49:29PM -0400, Tom Lane wrote:
>> Since prairiedog is not likely to be around forever, I propose that
>> we ought to enforce this going forward by arranging for common/logging.c
>> to not get built into the libpgcommon_shlib library variant.

> Agreed.  This makes sense in the long term, so attached is a proposal
> of patch.  I did not really see the point in complicating the MSVC
> scripts for that though.  The new variable names look natural to me
> like that, the new comment may need some tweaks.

Hm, it doesn't seem like "OBJS_PGCOMMON" conveys much.  I think
OBJS_FRONTEND ought to be the set of files built for frontend
application use (i.e., libpgcommon.a), so we need a new name
for what will go into libpgcommon_shlib.a.  Maybe OBJS_SHLIB,
or to be even more explicit, OBJS_FRONTEND_SHLIB.

As for the comment, maybe "logging.c is excluded from OBJS_SHLIB
as a matter of policy, because it is not appropriate for general
purpose libraries such as libpq to report errors directly."

            regards, tom lane

On Tue, Sep 29, 2020 at 09:29:18AM -0400, Tom Lane wrote:
> Hm, it doesn't seem like "OBJS_PGCOMMON" conveys much.  I think
> OBJS_FRONTEND ought to be the set of files built for frontend
> application use (i.e., libpgcommon.a), so we need a new name
> for what will go into libpgcommon_shlib.a.  Maybe OBJS_SHLIB,
> or to be even more explicit, OBJS_FRONTEND_SHLIB.

OBJS_SHLIB is already used to list the _shlib.o objects compiled with
libpgcommon_shlib.a, which is why I switched OBJS_FRONTEND to
OBJS_PGCOMMON, and an object cannot reference itself so we either need
two variables for the shlib list, or we could just do something like
that:
OBJS_FRONTEND = \
        $(OBJS_COMMON) \
    fe_memutils.o \
    restricted_token.o \
    sprompt.o
OBJS_SHLIB = $(OBJS_FRONTEND:%.o=%_shlib.o)
OBJS_FRONTEND += logging.o

However I would prefer the style of the attached.

> As for the comment, maybe "logging.c is excluded from OBJS_SHLIB
> as a matter of policy, because it is not appropriate for general
> purpose libraries such as libpq to report errors directly."

WFM.  Thanks!
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> However I would prefer the style of the attached.

LGTM.

            regards, tom lane

On Wed, Sep 30, 2020 at 03:47:26PM -0400, Tom Lane wrote:
> LGTM.

Thanks, I have applied this version then.  I am curious to see what
the buildfarm thinks about it.
--
Michael