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
diff --git a/src/common/Makefile b/src/common/Makefile index f281762885..d0bbaefb1e 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -88,13 +88,18 @@ OBJS_COMMON += sha2.o endif # A few files are currently only built for frontend, not server -# (Mkvcbuild.pm has a copy of this list, too) +# (Mkvcbuild.pm has a copy of this list, too). logging.c is +# explicitely excluded from the shared library as it enforces some +# global state used for error reporting, that should never be used +# by libpq or similar. OBJS_FRONTEND = \ $(OBJS_COMMON) \ fe_memutils.o \ - logging.o \ restricted_token.o \ sprompt.o +OBJS_PGCOMMON = \ + $(OBJS_FRONTEND) \ + logging.o # foo.o, foo_shlib.o, and foo_srv.o are all built from foo.c OBJS_SHLIB = $(OBJS_FRONTEND:%.o=%_shlib.o) @@ -121,7 +126,7 @@ uninstall: rm -f '$(DESTDIR)$(libdir)/libpgcommon.a' rm -f '$(DESTDIR)$(libdir)/libpgcommon_shlib.a' -libpgcommon.a: $(OBJS_FRONTEND) +libpgcommon.a: $(OBJS_PGCOMMON) rm -f $@ $(AR) $(AROPT) $@ $^ @@ -177,7 +182,7 @@ $(RYU_OBJS): CFLAGS += $(PERMIT_DECLARATION_AFTER_STATEMENT) # kwlist_d.h is in the distribution tarball, so it is not cleaned here. clean distclean: rm -f libpgcommon.a libpgcommon_shlib.a libpgcommon_srv.a - rm -f $(OBJS_FRONTEND) $(OBJS_SHLIB) $(OBJS_SRV) + rm -f $(OBJS_PGCOMMON) $(OBJS_SHLIB) $(OBJS_SRV) maintainer-clean: distclean rm -f kwlist_d.h
signature.asc
Description: PGP signature
