Thread: macos ventura SDK spews warnings

Hi

I had recently updated the M1 mini that I use to test macOS stuff on. Just
tried to test a change on it and was greeted with a lot of
warnings. Apparently the update brought in a newer SDK (MacOSX13.0.sdk), even
though the OS is still Monterey.

One class of warnings is specific to meson (see further down), but the other
is common between autoconf and meson:

[24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o
../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is deprecated: This function is provided for
compatibilityreasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended
thatyou use snprintf(3) instead. [-Wdeprecated-declarations]
 
        vallen = sprintf(convert, "%p", value);
                 ^
/Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/stdio.h:188:1: note: 'sprintf' has been explicitly
markeddeprecated here
 
__deprecated_msg("This function is provided for compatibility reasons only.  Due to security concerns inherent in the
designof sprintf(3), it is highly recommended that you use snprintf(3) instead.")
 
^
/Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/sys/cdefs.h:215:48: note: expanded from macro
'__deprecated_msg'
        #define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg)))
                                                      ^

the same warning is repeated for a bunch of different lines in the same file,
and then over the three versions of libpgport that we build.

This is pretty noisy.


The meson specific warning is
[972/1027] Linking target src/backend/replication/libpqwalreceiver/libpqwalreceiver.dylib
ld: warning: -undefined dynamic_lookup may not work with chained fixups

Which is caused by meson defaulting to -Wl,-undefined,dynamic_lookup for
modules. But we don't need that because we use -bund-loader. Adding
-Wl,-undefined,error as in the attached fixes it.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> One class of warnings is specific to meson (see further down), but the other
> is common between autoconf and meson:

> [24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o
> ../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is deprecated: This function is provided for
compatibilityreasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended
thatyou use snprintf(3) instead. [-Wdeprecated-declarations] 
>         vallen = sprintf(convert, "%p", value);
>                  ^

Originally we used the platform's sprintf there because we couldn't
rely on platforms having functional snprintf.  That's no longer the case,
I imagine, so we could just switch these calls over to snprintf.  I'm
kind of surprised that we haven't already been getting the likes of
this warning from, eg, OpenBSD.

Note that the hundreds of other sprintf calls in our code are actually
calling pg_sprintf, which hasn't got a deprecation label.  But the ones
in snprintf.c are really trying to call the platform's version.

(I wonder how much we ought to worry about bugs in the pg_sprintf usages?
But that's a matter for another day and another thread.)

            regards, tom lane

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> [24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o
>> ../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is deprecated: This function is provided for
compatibilityreasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended
thatyou use snprintf(3) instead. [-Wdeprecated-declarations] 

> Originally we used the platform's sprintf there because we couldn't
> rely on platforms having functional snprintf.  That's no longer the case,
> I imagine, so we could just switch these calls over to snprintf.  I'm
> kind of surprised that we haven't already been getting the likes of
> this warning from, eg, OpenBSD.

The attached seems enough to silence it for me.

Should we back-patch this?  I suppose, but how far?  It seems to fall
under the rules we established for back-patching into out-of-support
branches, ie it silences compiler warnings but shouldn't change any
behavior.  But it feels like a bigger change than most of the other
things we've done that with.

            regards, tom lane

diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index e037cf0a88..81d9c8c274 100644
--- a/src/port/snprintf.c
+++ b/src/port/snprintf.c
@@ -998,8 +998,8 @@ fmtptr(const void *value, PrintfTarget *target)
     int            vallen;
     char        convert[64];

-    /* we rely on regular C library's sprintf to do the basic conversion */
-    vallen = sprintf(convert, "%p", value);
+    /* we rely on regular C library's snprintf to do the basic conversion */
+    vallen = snprintf(convert, sizeof(convert), "%p", value);
     if (vallen < 0)
         target->failed = true;
     else
@@ -1149,11 +1149,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust,
     int            padlen;            /* amount to pad with spaces */

     /*
-     * We rely on the regular C library's sprintf to do the basic conversion,
+     * We rely on the regular C library's snprintf to do the basic conversion,
      * then handle padding considerations here.
      *
      * The dynamic range of "double" is about 1E+-308 for IEEE math, and not
-     * too wildly more than that with other hardware.  In "f" format, sprintf
+     * too wildly more than that with other hardware.  In "f" format, snprintf
      * could therefore generate at most 308 characters to the left of the
      * decimal point; while we need to allow the precision to get as high as
      * 308+17 to ensure that we don't truncate significant digits from very
@@ -1205,14 +1205,14 @@ fmtfloat(double value, char type, int forcesign, int leftjust,
             fmt[2] = '*';
             fmt[3] = type;
             fmt[4] = '\0';
-            vallen = sprintf(convert, fmt, prec, value);
+            vallen = snprintf(convert, sizeof(convert), fmt, prec, value);
         }
         else
         {
             fmt[0] = '%';
             fmt[1] = type;
             fmt[2] = '\0';
-            vallen = sprintf(convert, fmt, value);
+            vallen = snprintf(convert, sizeof(convert), fmt, value);
         }
         if (vallen < 0)
             goto fail;
@@ -1341,7 +1341,7 @@ pg_strfromd(char *str, size_t count, int precision, double value)
             fmt[2] = '*';
             fmt[3] = 'g';
             fmt[4] = '\0';
-            vallen = sprintf(convert, fmt, precision, value);
+            vallen = snprintf(convert, sizeof(convert), fmt, precision, value);
             if (vallen < 0)
             {
                 target.failed = true;

Hi,

On 2022-10-15 18:47:16 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> [24/2258] Compiling C object src/port/libpgport_srv.a.p/snprintf.c.o
> >> ../../../src/postgres/src/port/snprintf.c:1002:11: warning: 'sprintf' is deprecated: This function is provided for
compatibilityreasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended
thatyou use snprintf(3) instead. [-Wdeprecated-declarations]
 
> 
> > Originally we used the platform's sprintf there because we couldn't
> > rely on platforms having functional snprintf.  That's no longer the case,
> > I imagine, so we could just switch these calls over to snprintf.  I'm
> > kind of surprised that we haven't already been getting the likes of
> > this warning from, eg, OpenBSD.

Is there a platform still supported in older branches that we need to worry
about?


> The attached seems enough to silence it for me.
>
> Should we back-patch this?

Probably, but not sure either. We could just let it stew in HEAD for a while.


> I suppose, but how far?  It seems to fall under the rules we established for
> back-patching into out-of-support branches, ie it silences compiler warnings
> but shouldn't change any behavior.  But it feels like a bigger change than
> most of the other things we've done that with.

I wonder if we ought to add -Wno-deprecated to out-of-support branches to deal
with this kind of thing...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2022-10-15 18:47:16 -0400, Tom Lane wrote:
>>> Originally we used the platform's sprintf there because we couldn't
>>> rely on platforms having functional snprintf.  That's no longer the case,
>>> I imagine, so we could just switch these calls over to snprintf.

> Is there a platform still supported in older branches that we need to worry
> about?

snprintf is required by POSIX going back to SUSv2, so it's pretty darn
hard to imagine any currently-used platform that hasn't got it.  Even
my now-extinct dinosaur gaur had it (per digging in backup files).
I think we could certainly assume its presence in the branches that
require C99.  Even before that, is anybody really still building on
nineties-vintage platforms?

> I wonder if we ought to add -Wno-deprecated to out-of-support branches to deal
> with this kind of thing...

Yeah, that might be a better answer than playing whack-a-mole with
these sorts of warnings.

            regards, tom lane

On 2022-10-15 14:19:55 -0700, Andres Freund wrote:
> The meson specific warning is
> [972/1027] Linking target src/backend/replication/libpqwalreceiver/libpqwalreceiver.dylib
> ld: warning: -undefined dynamic_lookup may not work with chained fixups
> 
> Which is caused by meson defaulting to -Wl,-undefined,dynamic_lookup for
> modules. But we don't need that because we use -bund-loader. Adding
> -Wl,-undefined,error as in the attached fixes it.

Pushed the patch for that.

I wrote:
> snprintf is required by POSIX going back to SUSv2, so it's pretty darn
> hard to imagine any currently-used platform that hasn't got it.  Even
> my now-extinct dinosaur gaur had it (per digging in backup files).
> I think we could certainly assume its presence in the branches that
> require C99.

After further thought, I think the best compromise is just that:

(1) apply s/sprintf/snprintf/ patch in branches back to v12, where
we began to require C99.

(2) in v11 and back to 9.2, enable -Wno-deprecated if available.

One thing motivating this choice is that we're just a couple
weeks away from the final release of v10.  So I'm hesitant to do
anything that might turn out to be moving the portability goalposts
in v10.  But we're already assuming we can detect -Wno-foo options
correctly in v10 and older (e.g. 4c5a29c0e), so point (2) seems
pretty low-risk.

            regards, tom lane

Hi,

On 2022-10-15 21:00:00 -0400, Tom Lane wrote:
> I wrote:
> > snprintf is required by POSIX going back to SUSv2, so it's pretty darn
> > hard to imagine any currently-used platform that hasn't got it.  Even
> > my now-extinct dinosaur gaur had it (per digging in backup files).
> > I think we could certainly assume its presence in the branches that
> > require C99.
> 
> After further thought, I think the best compromise is just that:
> 
> (1) apply s/sprintf/snprintf/ patch in branches back to v12, where
> we began to require C99.
> 
> (2) in v11 and back to 9.2, enable -Wno-deprecated if available.
> 
> One thing motivating this choice is that we're just a couple
> weeks away from the final release of v10.  So I'm hesitant to do
> anything that might turn out to be moving the portability goalposts
> in v10.  But we're already assuming we can detect -Wno-foo options
> correctly in v10 and older (e.g. 4c5a29c0e), so point (2) seems
> pretty low-risk.

Makes sense to me.

- Andres

Andres Freund <andres@anarazel.de> writes:
> On 2022-10-15 21:00:00 -0400, Tom Lane wrote:
>> After further thought, I think the best compromise is just that:
>>
>> (1) apply s/sprintf/snprintf/ patch in branches back to v12, where
>> we began to require C99.
>>
>> (2) in v11 and back to 9.2, enable -Wno-deprecated if available.

> Makes sense to me.

I remembered another reason why v12 should be a cutoff: it's where
we started to use snprintf.c everywhere.  In prior branches, there'd
be a lot of complaints about sprintf elsewhere in the tree.

So I pushed (1), but on the way to testing (2), I discovered a totally
independent problem with the 13.0 SDK in older branches:

In file included from ../../../src/include/postgres.h:46:
In file included from ../../../src/include/c.h:1387:
In file included from ../../../src/include/port.h:17:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/netdb.h:91:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/netinet/in.h:81:
/Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/sys/socket.h:471:1: error: expected ';' after top
leveldeclarator 
__CCT_DECLARE_CONSTRAINED_PTR_TYPES(struct sockaddr_storage, sockaddr_storage);
^

This is apparently some sort of inclusion-order problem, which is probably
a bug in the SDK --- netdb.h and netinet/in.h are the same as they were in
SDK 12.3, but sys/socket.h has a few additions including this
__CCT_DECLARE_CONSTRAINED_PTR_TYPES macro, and evidently that's missing
something it needs.  I haven't traced down the cause of the problem yet.
It fails to manifest in v15 and HEAD, which I bisected to

98e93a1fc93e9b54eb477d870ec744e9e1669f34 is the first new commit
commit 98e93a1fc93e9b54eb477d870ec744e9e1669f34
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Tue Jan 11 13:46:12 2022 -0500

    Clean up messy API for src/port/thread.c.

    The point of this patch is to reduce inclusion spam by not needing
    to #include <netdb.h> or <pwd.h> in port.h (which is read by every
    compile in our tree).  To do that, we must remove port.h's
    declarations of pqGetpwuid and pqGethostbyname.

I doubt we want to back-patch that, so what we'll probably end up with
is adding some #includes to port.h in the back branches.  Bleah.
Or we could file a bug with Apple and hope they fix it quickly.
(They might, actually, because this SDK is supposedly beta.)

            regards, tom lane

I wrote:
> So I pushed (1), but on the way to testing (2), I discovered a totally
> independent problem with the 13.0 SDK in older branches:

> In file included from ../../../src/include/postgres.h:46:
> In file included from ../../../src/include/c.h:1387:
> In file included from ../../../src/include/port.h:17:
> In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/netdb.h:91:
> In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/netinet/in.h:81:
> /Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/sys/socket.h:471:1: error: expected ';' after top
leveldeclarator 
> __CCT_DECLARE_CONSTRAINED_PTR_TYPES(struct sockaddr_storage, sockaddr_storage);
> ^

Ah, I see it.  This is not failing everywhere, only in gram.y and
associated files, and it happens because those have a #define for REF,
which is breaking this constrained_ctypes stuff:

/Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/sys/socket.h:471:1: error: expected ';' after top
leveldeclarator 
__CCT_DECLARE_CONSTRAINED_PTR_TYPES(struct sockaddr_storage, sockaddr_storage);
^
/Library/Developer/CommandLineTools/SDKs/MacOSX13.0.sdk/usr/include/sys/constrained_ctypes.h:588:101: note: expanded
frommacro '__CCT_DECLARE_CONSTRAINED_PTR_TYPES' 
__CCT_DECLARE_CONSTRAINED_PTR_TYPE(basetype, basetag, REF);                                         \
                                                                                                    ^

Now on the one hand Apple is pretty clearly violating user namespace
by using a name like "REF", and I'll go file a bug about that.
On the other hand, #defining something like "REF" isn't very bright
on our part either.  We usually write something like REF_P when
there is a danger of parser tokens colliding with other names.

I think the correct, future-proof fix is to s/REF/REF_P/ in the
grammar.  We'll have to back-patch that, too, unless we want to
change what port.h includes.  I found that an alternative possible
band-aid is to do this in port.h:

diff --git a/src/include/port.h b/src/include/port.h
index b405d0e740..416428a0d2 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -14,7 +14,6 @@
 #define PG_PORT_H

 #include <ctype.h>
-#include <netdb.h>
 #include <pwd.h>

 /*
@@ -491,6 +490,8 @@ extern int  pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
                       size_t buflen, struct passwd **result);
 #endif

+struct hostent;                    /* avoid including <netdb.h> here */
+
 extern int     pqGethostbyname(const char *name,
                            struct hostent *resultbuf,
                            char *buffer, size_t buflen,

but it seems like there's a nonzero risk that some third-party
code somewhere is depending on our having included <netdb.h> here.
So ceasing to do that in the back branches doesn't seem great.

Changing a parser token name in the back branches isn't ideal
either, but it seems less risky to me than removing a globally
visible #include.

            regards, tom lane

I wrote:
> I think the correct, future-proof fix is to s/REF/REF_P/ in the
> grammar.

Done like that, after which I found that the pre-v12 branches are
compiling perfectly warning-free with the 13.0 SDK, despite nothing
having been done about sprintf.  This confused me mightily, but
after digging in Apple's headers I understand it.  What actually
gets provided by <stdio.h> is a declaration of sprintf(), now
with deprecation attribute attached, followed awhile later by

#if __has_builtin(__builtin___sprintf_chk) || defined(__GNUC__)
extern int __sprintf_chk (char * __restrict, int, size_t,
              const char * __restrict, ...);

#undef sprintf
#define sprintf(str, ...) \
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
#endif

So in the ordinary course of events, calling sprintf() results in
calling this non-deprecated builtin.  Only if you "#undef sprintf"
will you see the deprecation message.  snprintf.c does that, so
we see the message when that's built.  But if we don't use snprintf.c,
as the older branches do not on macOS, we don't ever #undef sprintf.

So for now, there seems no need for -Wno-deprecated, and I'm not
going to install it.

What I *am* seeing, in the 9.5 and 9.6 branches, is a ton of

ld: warning: -undefined dynamic_lookup may not work with chained fixups

apparently because we are specifying -Wl,-undefined,dynamic_lookup
which the other branches don't do.  That's kind of annoying,
but it looks like preventing that would be way too invasive :-(.
We'd added it to un-break some cases in the contrib transform
modules, and we didn't have a better solution until v10 [1].

            regards, tom lane

[1] https://www.postgresql.org/message-id/2652.1475512158%40sss.pgh.pa.us

Hi,

On 2022-10-16 16:45:24 -0400, Tom Lane wrote:
> I wrote:
> > I think the correct, future-proof fix is to s/REF/REF_P/ in the
> > grammar.
>
> Done like that, after which I found that the pre-v12 branches are
> compiling perfectly warning-free with the 13.0 SDK, despite nothing
> having been done about sprintf.  This confused me mightily, but
> after digging in Apple's headers I understand it.  What actually
> gets provided by <stdio.h> is a declaration of sprintf(), now
> with deprecation attribute attached, followed awhile later by
>
> #if __has_builtin(__builtin___sprintf_chk) || defined(__GNUC__)
> extern int __sprintf_chk (char * __restrict, int, size_t,
>               const char * __restrict, ...);
>
> #undef sprintf
> #define sprintf(str, ...) \
>   __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
> #endif
>
> So in the ordinary course of events, calling sprintf() results in
> calling this non-deprecated builtin.  Only if you "#undef sprintf"
> will you see the deprecation message.

Oh, huh. That's an odd setup... It's not like the the object size stuff
provides reliable protection.


> What I *am* seeing, in the 9.5 and 9.6 branches, is a ton of
>
> ld: warning: -undefined dynamic_lookup may not work with chained fixups
>
> apparently because we are specifying -Wl,-undefined,dynamic_lookup
> which the other branches don't do.  That's kind of annoying,
> but it looks like preventing that would be way too invasive :-(.
> We'd added it to un-break some cases in the contrib transform
> modules, and we didn't have a better solution until v10 [1].

Hm - I think it might actually mean that transforms won't work with the new
macos relocation format, which is what I understand "chained fixups" to be.

Unfortunately it looks like the chained fixup stuff is enabled even when
targetting Monterey, even though it was only introduced with the 13 sdk (I
think).  But it does look like using the macos 11 SDK sysroot does force the
use of the older relocation format. So we have at least some way out :/

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2022-10-16 16:45:24 -0400, Tom Lane wrote:
>> What I *am* seeing, in the 9.5 and 9.6 branches, is a ton of
>> 
>> ld: warning: -undefined dynamic_lookup may not work with chained fixups
>> 
>> apparently because we are specifying -Wl,-undefined,dynamic_lookup
>> which the other branches don't do.  That's kind of annoying,
>> but it looks like preventing that would be way too invasive :-(.
>> We'd added it to un-break some cases in the contrib transform
>> modules, and we didn't have a better solution until v10 [1].

> Hm - I think it might actually mean that transforms won't work with the new
> macos relocation format, which is what I understand "chained fixups" to be.

Hm ... hstore_plpython and ltree_plpython still pass regression check in
9.6, so it works for at least moderate-size values of "work", at least on
Monterey.  But in any case, if there's a problem there I can't see us
doing anything about that in dead branches.  The "keep it building" rule
doesn't extend to perl or python dependencies IMO.

            regards, tom lane