networkupstools / nut

The Network UPS Tools repository. UPS management protocol Informational RFC 9271 published by IETF at https://www.rfc-editor.org/info/rfc9271 Please star NUT on GitHub, this helps with sponsorships!
https://networkupstools.org/
Other
2k stars 349 forks source link

compiler warning: printf allows null string args... no (new/broken check?) #2109

Closed desertwitch closed 12 months ago

desertwitch commented 12 months ago

Compiling on my usual (unchanged) build environment on Slackware 15.0, gcc 11.2.0:

last build from 3bf526e72332d01a6242a87188ba21c2fdc628ab

checking for practical support to printf("%s", NULL)... yes
checking whether printf allows null string args... (null)ok

new build from current master (after 3bf526e72332d01a6242a87188ba21c2fdc628ab)

checking for practical support to printf("%s", NULL)... yes
checking whether printf allows null string args... no
configure: WARNING: Your C library requires workarounds to print NULL values; if something crashes with a segmentation fault (especially with verbose debug) - that may be why

Attempt to re-create the check on my build system:

#include <stdlib.h>  
#include <stdio.h>
int main() 
{ 
   printf("%s", NULL); 
   return 0;
} 
(null)

I was trying to pin-point this from the commits since, but I couldn't figure out what has changed that could cause this.

jimklimov commented 12 months ago

No, nothing was changed recently that should have impacted this...

Is it a cross-build or native (so executables can run locally)? Can something like an antivirus get in the way of running test programs, having their newly written binaries locked for its own checks - breaks a lot of active logic on Windows, for example.

The autotools config.log should have a detailed log of what it ran and what errors it got. It is usually helpful...

desertwitch commented 12 months ago

No, nothing was changed recently that should have impacted this...

Is it a cross-build or native (so executables can run locally)? Can something like an antivirus get in the way of running test programs, having their newly written binaries locked for its own checks - breaks a lot of active logic on Windows, for example.

The autotools config.log should have a detailed log of what it ran and what errors it got. It is usually helpful...

It's a native build on Slackware 15.0 with gcc 11.2.0 No antivirus or the likes, the build environment is entirely unchanged.

Actually I've checked-out 3bf526e72332d01a6242a87188ba21c2fdc628ab again just now and built it with the null arg check passing. Afterwards I've checked-out the latest master state and built with the null arg check failing.

desertwitch commented 12 months ago

Here goes config.log building from current master just now:

configure:14136: checking whether printf allows null string args
configure:14214: x86_64-slackware-linux-gcc -o conftest -isystem /usr/local/include -g -O2 -Wno-reserved-identifier -Wno-unknown-warning-option -std=gnu99 -Werror -Werror=implicit-function-declaration   conftest.c  >&5
conftest.c: In function 'main':
conftest.c:139:1: error: implicit declaration of function 'printf' [-Werror=implicit-function-declaration]
  139 | printf("%s", NULL)
      | ^~~~~~
conftest.c:136:1: note: include '<stdio.h>' or provide a declaration of 'printf'
  135 | #include <stdlib.h>
  +++ |+#include <stdio.h>
  136 | int
conftest.c:139:1: error: incompatible implicit declaration of built-in function 'printf' [-Werror=builtin-declaration-mismatch]
  139 | printf("%s", NULL)
      | ^~~~~~
conftest.c:139:1: note: include '<stdio.h>' or provide a declaration of 'printf'
conftest.c: At top level:
cc1: note: unrecognized command-line option '-Wno-unknown-warning-option' may have been intended to silence earlier diagnostics
cc1: note: unrecognized command-line option '-Wno-reserved-identifier' may have been intended to silence earlier diagnostics
cc1: all warnings being treated as errors
configure:14214: $? = 1
configure: program exited with status 1
configure: failed program was:
| /* confdefs.h */

Here goes config.log building from 3bf526e72332d01a6242a87188ba21c2fdc628ab just now:

configure:13949: checking whether printf allows null string args
configure:14023: x86_64-slackware-linux-gcc -o conftest -isystem /usr/local/include -g -O2 -Wno-reserved-identifier -Wno-unknown-warning-option -std=gnu99   conftest.c  >&5
conftest.c: In function 'main':
conftest.c:139:1: warning: implicit declaration of function 'printf' [-Wimplicit-function-declaration]
  139 | printf("%s", NULL)
      | ^~~~~~
conftest.c:136:1: note: include '<stdio.h>' or provide a declaration of 'printf'
  135 | #include <stdlib.h>
  +++ |+#include <stdio.h>
  136 | int
conftest.c:139:1: warning: incompatible implicit declaration of built-in function 'printf' [-Wbuiltin-declaration-mismatch]
  139 | printf("%s", NULL)
      | ^~~~~~
conftest.c:139:1: note: include '<stdio.h>' or provide a declaration of 'printf'
conftest.c: At top level:
cc1: note: unrecognized command-line option '-Wno-unknown-warning-option' may have been intended to silence earlier diagnostics
cc1: note: unrecognized command-line option '-Wno-reserved-identifier' may have been intended to silence earlier diagnostics
configure:14023: $? = 0
configure:14023: ./conftest
configure:14023: $? = 0
configure:14025: result: ok

Seems the warning in the "working" build was raised to be an error in the newer master. cc1: all warnings being treated as errors

desertwitch commented 12 months ago

Seems I found it - error promotion comes from this change https://github.com/networkupstools/nut/commit/5d1236b52cf2b2c72a4560803018f087b2029da4 in https://github.com/networkupstools/nut/pull/2096

jimklimov commented 12 months ago

Interesting, seems the checking code snippet defined in configure.ac was not complete, then, thanks.

jimklimov commented 12 months ago

Good catch - yes, that change was intentional as we had some other tests sort-of-passing when something was not really supported. But did not cross-check if some other new "errors" got discovered that way.

desertwitch commented 12 months ago

I'm the one who's thankful for the effort you put into all of this - so, thanks ;-)

jimklimov commented 12 months ago

So, after running some log collections with the likes of make distclean ; ./autogen.sh && ./configure --with-all 2>&1 | tee configure-9cf0811.log && cp -pf config{,-9cf0811}.log, the diffs are:

$ diff -bu configure-*.log | egrep '^[+-]'
--- configure-3bf526e.log       2023-10-16 10:06:05.955989071 +0200
+++ configure-9cf0811.log       2023-10-16 10:07:51.340478089 +0200
-Network UPS Tools version 2.8.0.1 (v2.8.0-signed-2608-g3bf526e72)
+Network UPS Tools version 2.8.0.1 (v2.8.0-signed-2667-g9cf0811d7)
+checking for pragma GCC diagnostic ignored "-Wcast-function-type-strict"... no
+checking for pragma GCC diagnostic ignored "-Wcast-function-type-strict" (outside functions)... no
-checking for practical support to printf("%s", NULL)... yes
+checking for practical support to printf("%s", NULL)... no
-checking whether printf allows null string args... (null)ok
+checking whether printf allows null string args... no
+configure: WARNING: Your C library requires workarounds to print NULL values; if something crashes with a segmentation fault (especially with verbose debug) - that may be why
+checking for inet_pton() with IPv4 and IPv6 support... 11yes
+checking for struct pollfd... yes
-checking if your systemd version supports Type=notify-reload... /home/jim/nut/systemd-analyze-ebCdQG.service:5: Failed to parse service type, ignoring: notify-reload
+checking if your systemd version supports Type=notify-reload... /home/jim/nut/systemd-analyze-gmrEmj.service:5: Failed to parse service type, ignoring: notify-reload
-checking if current toolkit can build and run cppunit tests (e.g. no ABI issues, related segfaults, etc.)... .
-
-yes
-checking for impact from --enable-cppunit option - should we build cppunit tests?... yes
-checking whether to build C++ tests with CPPUNIT... yes
+checking if current toolkit can build and run cppunit tests (e.g. no ABI issues, related segfaults, etc.)... no
+checking for impact from --enable-cppunit option - should we build cppunit tests?... no
+checking whether to build C++ tests with CPPUNIT... no
-* configured version:  2.8.0.1 (v2.8.0-signed-2608-g3bf526e72)
+* configured version:  2.8.0.1 (v2.8.0-signed-2667-g9cf0811d7)
-* build C++ tests with CPPUNIT:        yes
+* build C++ tests with CPPUNIT:        no

So it apparently also "lost" cppunit tests. Thanks again :)

desertwitch commented 12 months ago

Can confirm this works as intended when I patch in an inclusion of stdio.h:

diff --git a/configure.ac b/configure.ac
index 66821dde4..3e9977828 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1030,7 +1030,8 @@ dnl The following code crashes on some libc implementations (e.g. Solaris 8)
 dnl TODO: We may need to update NUT codebase to use NUT_STRARG() macro more
 dnl often and consistently, or find a way to tweak upsdebugx() etc. varargs
 AX_RUN_OR_LINK_IFELSE([AC_LANG_PROGRAM(
-    [#include <stdlib.h>],
+    [#include <stdlib.h>
+#include <stdio.h>],
     [printf("%s", NULL)]
 )],
     [
configure:14136: checking whether printf allows null string args
configure:14216: x86_64-slackware-linux-gcc -o conftest -isystem /usr/local/include -g -O2 -Wno-reserved-identifier -Wno-unknown-warning-option -std=gnu99 -Werror -Werror=implicit-function-declaration   conftest.c  >&5
configure:14216: $? = 0
configure:14216: ./conftest
configure:14216: $? = 0
configure:14218: result: ok
jimklimov commented 12 months ago

Huh, it seems there are two tests historically - in the main configure.ac script and in m4:

jimklimov commented 12 months ago

So with a fix like yours, it still rejects the change for my test system under hands (WSL2 Ubuntu 22.04, gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0):

configure:15546: checking whether printf allows null string args
configure:15632: gcc -o conftest -isystem /usr/local/include -g -O2 -Wno-reserved-identifier -Wno-unknown-warning-option -std=gnu99 -Werror -Werror=implicit-function-declaration   conftest.c  >&5
conftest.c: In function 'main':
conftest.c:152:10: error: format '%s' expects argument of type 'char *', but argument 2 has type 'void *' [-Werror=format=]
  152 | printf("%s", NULL)
      |         ~^
      |          |
      |          char *
      |         %p
In file included from /usr/include/stdio.h:894,
                 from conftest.c:147:
In function 'printf',
    inlined from 'main' at conftest.c:152:1:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:112:10: error: '%s' directive argument is null [-Werror=format-overflow=]
  112 |   return __printf_chk (__USE_FORTIFY_LEVEL - 1, __fmt, __va_arg_pack ());
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
conftest.c: At top level:
cc1: note: unrecognized command-line option '-Wno-unknown-warning-option' may have been intended to silence earlier diagnostics
cc1: note: unrecognized command-line option '-Wno-reserved-identifier' may have been intended to silence earlier diagnostics
cc1: all warnings being treated as errors
configure:15632: $? = 1
configure: program exited with status 1
configure: failed program was:
...
| /* end confdefs.h.  */
|
| #include <stdlib.h>
| #include <stdio.h>
|
| int
| main (void)
| {
| printf("%s", NULL)
|
|   ;
|   return 0;
| }
configure:15647: result: no
configure:15652: WARNING: Your C library requires workarounds to print NULL values; if something crashes with a segmentation fault (especially with verbose debug) - that may be why

Both tests seem to fail with master-branch codebase in fact:

-checking for practical support to printf("%s", NULL)... yes
+checking for practical support to printf("%s", NULL)... no
-checking whether printf allows null string args... (null)ok
+checking whether printf allows null string args... no

In the older version, the m4 test did complain (warnings about NULL) but did "succeed" as far as practice goes (despite complaints, current compiler+libc do provide a fallback):

configure:13418: checking for practical support to printf("%s", NULL)
configure:13506: gcc -o conftest -isystem /usr/local/include -g -O2 -Wno-reserved-identifier -Wno-unknown-warning-option -std=gnu99   conftest.c  >&5
In file included from /usr/include/stdio.h:894,
                 from conftest.c:90:
In function 'snprintf',
    inlined from 'main' at conftest.c:99:11:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:71:10: warning: '%s' directive argument is null [-Wformat-truncation=]
   71 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   72 |                                    __glibc_objsize (__s), __fmt,
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   73 |                                    __va_arg_pack ());
      |                                    ~~~~~~~~~~~~~~~~~
conftest.c: At top level:
cc1: note: unrecognized command-line option '-Wno-unknown-warning-option' may have been intended to silence earlier diagnostics
cc1: note: unrecognized command-line option '-Wno-reserved-identifier' may have been intended to silence earlier diagnostics
configure:13506: $? = 0
configure:13506: ./conftest
SUCCESS: RETURNED a string that contains something like 'null' from snprintf() with a NULL string argument: '(null)'
configure:13506: $? = 0
configure:13520: result: yes

So I guess I need a way to pass -Wno-... into this link/run call to allow for NULL if it works practically, and to squash the two implementations :)

For context, this is part of the bugfix effort that introduced NUT_STRARG (on some systems, upsdebugx() printouts segfaulted due to NULL string parameters) but it is far from being used everywhere yet.

desertwitch commented 12 months ago

Interesting, the m4 test always passed for me, the configure.ac test now passes (but only with the include patched in).