vasnprintf.c vs GCC11's -Wanalyzer-null-argument (and glibc-2.31)

vasnprintf.c vs GCC11's -Wanalyzer-null-argument (and glibc-2.31)

[Jim Meyering]

[-- Attachment #1: Type: text/plain, Size: 165 bytes --]

I built latest gcc from git and then attempted to build things with it.
I noticed this new warning while trying to build diffutils, but
haven't had time to address:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vasnprintf-warning.txt --]
[-- Type: text/plain; charset="GB18030"; name="vasnprintf-warning.txt", Size: 5422 bytes --]

Making all in lib
make[1]: Entering directory '/home/j/w/co/diffutils/lib'
make  all-am
make[2]: Entering directory '/home/j/w/co/diffutils/lib'
  CC       vasnprintf.o
In file included from /usr/include/string.h:495,
                 from ./string.h:41,
                 from vasnprintf.c:79:
vasnprintf.c: In function 'vasnprintf':
/usr/include/bits/string_fortified.h:34:10: error: use of NULL 'result' where non-null expected [CWE-476] [-Werror=analyzer-null-argument]
   34 |   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  'vasnprintf': events 1-4
    |
    |vasnprintf.c:1858:6:
    | 1858 |   if (PRINTF_PARSE (format, &d, &a) < 0)
    |      |      ^
    |      |      |
    |      |      (1) following 'false' branch...
    |......
    | 1868 |   if (PRINTF_FETCHARGS (args, &a) < 0)
    |      |   ~~ ~
    |      |   |  |
    |      |   |  (3) following 'false' branch...
    |      |   (2) ...to here
    |......
    | 1876 |     size_t buf_neededlength;
    |      |     ~~~~~~
    |      |     |
    |      |     (4) ...to here
    |
  'vasnprintf': events 5-6
    |
    |xsize.h:66:30:
    |   66 |   return (sum >= size1 ? sum : SIZE_MAX);
    |      |          ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
    |      |                              |
    |      |                              (5) following 'true' branch...
    |......
    |   80 |   return xsum (xsum (xsum (size1, size2), size3), size4);
    |      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |          |
    |      |          (6) ...to here
    |
  'vasnprintf': events 7-12
    |
    |vasnprintf.c:1892:8:
    | 1892 |     if (buf_neededlength < 4000 / sizeof (TCHAR_T))
    |      |        ^
    |      |        |
    |      |        (7) following 'true' branch (when 'sum <= 3999')...
    | 1893 |       {
    | 1894 |         buf = (TCHAR_T *) alloca (buf_neededlength * sizeof (TCHAR_T));
    |      |         ~~~
    |      |         |
    |      |         (8) ...to here
    |......
    | 1909 |     if (resultbuf != NULL)
    |      |        ~
    |      |        |
    |      |        (9) following 'false' branch (when 'resultbuf' is NULL)...
    |......
    | 1919 |     length = 0;
    |      |     ~~~~~~
    |      |     |
    |      |     (10) ...to here
    |......
    | 1951 |         if (cp != dp->dir_start)
    |      |            ~
    |      |            |
    |      |            (11) following 'true' branch...
    | 1952 |           {
    | 1953 |             size_t n = dp->dir_start - cp;
    |      |             ~~~~~~
    |      |             |
    |      |             (12) ...to here
    |
  'vasnprintf': event 13
    |
    |xsize.h:66:30:
    |   66 |   return (sum >= size1 ? sum : SIZE_MAX);
    |      |          ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
    |      |                              |
    |      |                              (13) following 'true' branch (when 'length <= sum')...
    |
  'vasnprintf': event 14
    |
    |vasnprintf.c:1954:39:
    | 1954 |             size_t augmented_length = xsum (length, n);
    |      |                                       ^~~~~~~~~~~~~~~~
    |      |                                       |
    |      |                                       (14) ...to here
    |
  'vasnprintf': event 15
    |
    | 1927 |     if ((needed) > allocated)                                                \
    |      |        ^
    |      |        |
    |      |        (15) following 'false' branch...
vasnprintf.c:1956:13: note: in expansion of macro 'ENSURE_ALLOCATION'
    | 1956 |             ENSURE_ALLOCATION (augmented_length);
    |      |             ^~~~~~~~~~~~~~~~~
    |
  'vasnprintf': event 16
    |
    |  151 | #  define DCHAR_CPY memcpy
    |      |                     ^
    |      |                     |
    |      |                     (16) ...to here
vasnprintf.c:1945:11: note: in expansion of macro 'DCHAR_CPY'
    | 1945 |           DCHAR_CPY (memory, result, length);                                \
    |      |           ^~~~~~~~~
vasnprintf.c:1956:13: note: in expansion of macro 'ENSURE_ALLOCATION'
    | 1956 |             ENSURE_ALLOCATION (augmented_length);
    |      |             ^~~~~~~~~~~~~~~~~
    |
  'vasnprintf': events 17-18
    |
    | 1962 |                 DCHAR_CPY (result + length, (const DCHAR_T *) cp, n);
    |      |                            ~~~~~~~^~~~~~~~
    |      |                                   |
    |      |                                   (17) 'result' is NULL
    |      |                                   (18) 'result' is NULL
    |
  'vasnprintf': event 19
    |
    |/usr/include/bits/string_fortified.h:34:10:
    |   34 |   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
    |      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |          |
    |      |          (19) argument 1 ('<unknown>') NULL where non-null expected
    |
<built-in>: note: argument 1 of '__builtin___memcpy_chk' must be non-null
cc1: all warnings being treated as errors
make[2]: *** [Makefile:2240: vasnprintf.o] Error 1
make[2]: Leaving directory '/home/j/w/co/diffutils/lib'
make[1]: *** [Makefile:1969: all] Error 2
make[1]: Leaving directory '/home/j/w/co/diffutils/lib'
make: *** [Makefile:1626: all-recursive] Error 1
\a\a\a

Re: vasnprintf.c vs GCC11's -Wanalyzer-null-argument (and glibc-2.31)

[Bruno Haible]

Hi Jim,

> I built latest gcc from git and then attempted to build things with it.
> I noticed this new warning while trying to build diffutils, but
> haven't had time to address:

The vast majority of -Wanalyzer* warnings that we have seen so far were false
alarms [1].

This one is a false alarm as well. The compiler is complaining that in
vasnprintf.c:1962

        if (cp != dp->dir_start)                                       // <== 1951
          {
            size_t n = dp->dir_start - cp;                             // <== 1953
            size_t augmented_length = xsum (length, n);                // <== 1954

            ENSURE_ALLOCATION (augmented_length);                      // <== 1956
            /* This copies a piece of FCHAR_T[] into a DCHAR_T[].  Here we
               need that the format string contains only ASCII characters
               if FCHAR_T and DCHAR_T are not the same type.  */
            if (sizeof (FCHAR_T) == sizeof (DCHAR_T))
              {
                DCHAR_CPY (result + length, (const DCHAR_T *) cp, n);  // <== 1962

DCHAR_CPY = memcpy could be invoked with result = NULL. But when you look
at all assignments to 'result', result is only assigned NULL in line 1916,
and here allocated = 0. By the logic of the ENSURE_ALLOCATION macro, 'result'
is only assigned a new value when 'allocated' is, and vice versa.
Now, since in line 1953 n > 0 (due to line 1951), in line 1954 augmented_length
is also > 0 (by the definition of xsum), hence ENSURE_ALLOCATION in line 1956
gets invoked with a positive argument, thus in line 1927
   if ((needed) > allocated)
must evaluate to true. The compiler's logic did not see this; it proposed
a code path where in line 1927 the condition evaluates to false — which
cannot happen.

So, in the current state, looking at -fanalyzer / -Wanalyzer outputs is
still a waste of time.

I don't want to modify the source code to silence false alarms from this
(yet immature) tool.

Bruno

[1] https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00118.html

Re: vasnprintf.c vs GCC11's -Wanalyzer-null-argument (and glibc-2.31)

[Bruno Haible]

When I wrote:
> So, in the current state, looking at -fanalyzer / -Wanalyzer outputs is
> still a waste of time.

I meant this with respect to Gnulib source code, which is usually reviewed
by 2 people, which has unit tests, and so on.

It is well possible that -fanalyzer / -Wanalyzer is useful to find simple
coding mistakes in other packages.

Bruno

Re: vasnprintf.c vs GCC11's -Wanalyzer-null-argument (and glibc-2.31)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

On 1/2/21 5:49 PM, Bruno Haible wrote:
> The vast majority of -Wanalyzer* warnings that we have seen so far were false
> alarms [1].

I've had such bad luck with those warnings that I have not been much 
motivated to file GCC bug reports for them. I guess the warnings are 
helpful with low-quality code, but I think I've only found one bug with 
them in many months of using them on several GNU projects, as compared 
to a lot of false alarms. I'm almost tempted to disable them in Gnulib 
by default.

For diffutils I worked around the problem by installing the attached 
patch, which disables the warning in Gnulib code.

Without the attached patch I got the same warning that Jim got, when I 
used GCC 10.2.1 20201125 (Red Hat 10.2.1-9) x86-64. I got more warnings 
elsewhere in Gnulib when I used gcc (Ubuntu 10.2.0-13ubuntu1) 10.2.0 
x86-64, but I'd rather not work around those bugs as we can just ask 
people to use --disable-gcc-warnings if their GCC is old.

[-- Attachment #2: 0001-maint-work-around-GCC-Wreturn-local-addr-bug.patch --]
[-- Type: text/x-patch, Size: 1046 bytes --]

From f88b033174c4fa816d3b146312820725c658f707 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 2 Jan 2021 19:07:53 -0800
Subject: [PATCH] maint: work around GCC -Wreturn-local-addr bug

* configure.ac: Do not use -Wreturn-local-addr in Gnulib,
to suppress a false alarm in vasnprintf.c.
---
 configure.ac | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure.ac b/configure.ac
index ef01345..3b2195e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -95,6 +95,10 @@ if test "$gl_gcc_warnings" = yes; then
   nw="$nw -Wsuggest-attribute=pure"
   nw="$nw -Wduplicated-branches"    # Too many false alarms
 
+  # Avoid false alarm in lib/vasnprintf.c.
+  # https://lists.gnu.org/r/bug-gnulib/2021-01/msg00031.html
+  gl_WARN_ADD([-Wno-analyzer-null-argument])
+
   gl_WARN_ADD([-Wno-return-local-addr])    # avoid this false alarm:
   # careadlinkat.c: In function 'careadlinkat':
   # cc1: error: function may return address of local variable [-Werror=return-local-addr]
-- 
2.27.0

Re: vasnprintf.c vs GCC11's -Wanalyzer-null-argument (and glibc-2.31)

[Bruno Haible]

Paul Eggert wrote:
> I'd rather not work around those bugs as we can just ask 
> people to use --disable-gcc-warnings if their GCC is old.

Yes. Or to set CPPFLAGS="-fno-analyzer" with GCC >= 10.

Bruno

Re: vasnprintf.c vs GCC11's -Wanalyzer-null-argument (and glibc-2.31)

[Jim Meyering]

On Sat, Jan 2, 2021 at 7:22 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 1/2/21 5:49 PM, Bruno Haible wrote:
> > The vast majority of -Wanalyzer* warnings that we have seen so far were false
> > alarms [1].
>
> I've had such bad luck with those warnings that I have not been much
> motivated to file GCC bug reports for them. I guess the warnings are
> helpful with low-quality code, but I think I've only found one bug with
> them in many months of using them on several GNU projects, as compared
> to a lot of false alarms. I'm almost tempted to disable them in Gnulib
> by default.
>
> For diffutils I worked around the problem by installing the attached
> patch, which disables the warning in Gnulib code.
>
> Without the attached patch I got the same warning that Jim got, when I
> used GCC 10.2.1 20201125 (Red Hat 10.2.1-9) x86-64. I got more warnings
> elsewhere in Gnulib when I used gcc (Ubuntu 10.2.0-13ubuntu1) 10.2.0
> x86-64, but I'd rather not work around those bugs as we can just ask
> people to use --disable-gcc-warnings if their GCC is old.

Thanks to both of you for the quick work/feedback Sorry I must agree
it's best to disable -- though I would have been tempted to disable it
only for that one file, rather than for all of gnulib that diffutils
will ever use. I do admit the difference is minimal, given gnulib's
maturity and test coverage.