gcc12 static analyzer (-fanalyzer) report

[chipitsine]

Tool Name and Version

gcc12

Code Report

I tested on Fedora Rawhide with `-fanalyzer` enabled:


src/ssl_sample.c: In function 'smp_fetch_ssl_fc_has_early':
src/ssl_sample.c:484:34: error: dereference of NULL '0' [CWE-476] [-Werror=analyzer-null-dereference]
  484 |         smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA)  &&
      |                              ~~~~^~~~~~~
compilation terminated due to -Wfatal-errors.
cc1: all warnings being treated as errors
make: *** [Makefile:1004: src/ssl_sample.o] Error 1
make: *** Waiting for unfinished jobs....
src/ssl_crtlist.c: In function 'crtlist_dup_filters':
src/ssl_crtlist.c:182:20: error: leak of 'strdup(*args_17(D) + _4)' [CWE-401] [-Werror=analyzer-malloc-leak]
  182 |                 if (!dst[i])
      |                    ^
compilation terminated due to -Wfatal-errors.
cc1: all warnings being treated as errors
make: *** [Makefile:1004: src/ssl_crtlist.o] Error 1
In file included from include/haproxy/session.h:26,
                 from include/haproxy/stream.h:32,
                 from include/haproxy/channel.h:30,
                 from include/haproxy/applet.h:29,
                 from src/ssl_sock.c:47:
In function 'conn_ctrl_ready',
    inlined from 'ssl_sock_handshake' at src/ssl_sock.c:5893:7:
include/haproxy/connection.h:118:21: error: dereference of NULL 'conn' [CWE-476] [-Werror=analyzer-null-dereference]
  118 |         return (conn->flags & CO_FL_CTRL_READY);
      |                 ~~~~^~~~~~~

### Additional Information

_No response_

### Output of `haproxy -vv`

```plain
no

[wtarreau]

The first one is wrong but the compiler cannot know it, we ought to use __objt_conn() instead.

The second is valid, the loop ought to be rolled back first.

The third looks wrong. We're at the function entry and this function cannot be called with a NULL. I suspect the analyzer believes this because before that point we call a generic function that performs a null test, but if it relies on this, it's plain stupid and will constantly report tons of false positives.

[chipitsine]

well, I'll try to dig on third

[chipitsine]

in ssl_sock_handshake we invoke conn_get_ssl_sock_ctx which compares conn against NULL. thus gcc assumes conn might be NULL.

after that we invoke conn_ctrl_ready without prior check for NULL

static int ssl_sock_handshake(struct connection *conn, unsigned int flag)
{
	struct ssl_sock_ctx *ctx = conn_get_ssl_sock_ctx(conn);
	int ret;
	struct ssl_counters *counters = NULL;
	struct ssl_counters *counters_px = NULL;
	struct listener *li;
	struct server *srv;
	socklen_t lskerr;
	int skerr;


	if (!conn_ctrl_ready(conn))
		return 0;

[chipitsine]

can we address first and second finding ?

[chipitsine]

after I masked out 3rd detection, another detection revealed

cc1: all warnings being treated as errors
make: *** [Makefile:982: src/ssl_ckch.o] Error 1
src/ssl_sock.c: In function ‘ssl_sock_io_cb’:
src/ssl_sock.c:6241:41: error: dereference of NULL ‘0’ [CWE-476] [-Werror=analyzer-null-dereference]
 6241 |                         eb64_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node);
      |                                      ~~~^~~~~~~~~
compilation terminated due to -Wfatal-errors.

[wtarreau]

Ilya, in order for me not to waste an hour trying to reproduce, could you please share your build options so that it's easier to try here ? It's indeed extremely difficult to shut absurd warnings when you cannot reproduce them because they're usually everything but rational.

[chipitsine]

I use gcc from master branch. However, static analyzer did not change a lot from gcc-12.X

#!/bin/bash

export CC=/home/ilia/gcc/gcc-home/bin/gcc
make clean
make TARGET=linux-glibc USE_OPENSSL=1 DEBUG_CFLAGS="-fanalyzer"

[wtarreau]

Thank you. I'll see if I find a working compiler.

[chipitsine]

I reported 3 issues because I first invoked make -j3 ERR=1... and it stopped after 3 detections

[wtarreau]

I can see the first one using the same flags and gcc-11.3. It's completely bogus as usual. It considers that because you call a general-purpose function on one of your pointers and that this function performs safety checks so that it can be used in other places, then it necessarily means that where you call it those safety checks may always be violated. It's as if it reported that a string might be NULL everywhere you call printf("%p") just because printf() does an explicit test to print "(nil)" instead of "0x0". In my opinion such warnings only ought to be emitted if the test is explicit in the function and not if it comes from another function (or macro), inlined or not, since the main purpose of writing a function is to reuse it.

Such absurdities are causing great damage to the C language because they make it more and more complicated to use safely, and discourage users from reusing valid code and instead for them to reimplement copies of it everywhere. Several times already we introduced bugs while trying to shut the bogus null-deref checks up :-( And that's sad because when they're done properly (i.e. local function only) they sometimes report valid issues.

I can shut it down using DISGUISE() when calling the function. That's ugly... For the other ones I don't know, I'll have to check.

[shipitsin-ilia]

I'm not sure that we can report to gcc "please ignore some checks, and honor other". Looks like those "null deref" checks are totally useless.

we can try to enable gcc static analyzer with null deref muted for example (at least for the first time)

[wtarreau]

I got another one which is not even possible in smp_fetch_ssl_fc_has_early():

src/ssl_sample.c:502:34: warning: dereference of NULL '0' [CWE-476] [-Wanalyzer-null-dereference]
  502 |         smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA)  &&
      |                              ~~~~^~~~~~~
  'smp_fetch_ssl_fc_has_early': events 1-3
    |
    |  491 |         if (!ssl)
    |      |            ^
    |      |            |
    |      |            (1) following 'false' branch (when 'ssl' is non-NULL)...
    |......
    |  494 |         smp->flags = 0;
    |      |         ~~~~~~~~~~~~~~
    |      |                    |
    |      |                    (2) ...to here
    |......
    |  502 |         smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA)  &&
    |      |                              ~~~~~~~~~~~
    |      |                                  |
    |      |                                  (3) dereference of NULL '<unknown>'
    |

The two lines before the "if" explicitly make sure that a NULL conn will cause ssl to be NULL, so we can't pass beyond that if (!ssl).

Worse, even if I add an explicit test in the if condition above for NULL conn, it still manages to report me two explicitly conflicting conditions (this one is awesome, really):

src/ssl_sample.c:502:34: warning: dereference of NULL '0' [CWE-476] [-Wanalyzer-null-dereference]
  502 |         smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA)  &&
      |                              ~~~~^~~~~~~
  'smp_fetch_ssl_fc_has_early': events 1-3
    |
    |  491 |         if (!conn || !ssl)
    |      |            ^
    |      |            |
    |      |            (1) following 'false' branch...
    |......
    |  494 |         smp->flags = 0;
    |      |         ~~~~~~~~~~~~~~
    |      |                    |
    |      |                    (2) ...to here
    |......
    |  502 |         smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA)  &&
    |      |                              ~~~~~~~~~~~
    |      |                                  |
    |      |                                  (3) dereference of NULL '<unknown>'
    |

It explicitly says that conn is not NULL, just to say a few lines below that it was NULL.

At this point it's clear that this analyzer is broken beyond imagination. It might be extremely experimental, which is fine if that's the case. But we can't sabottage all the code with invalid "if" just to try to shut it up. If it says garbage, we should just shut it up or stop using it. Because I'm not going to purposely add bugs in the code to hide the analyzer's bugs.

[wtarreau]

Found it, you can disable it with: -Wno-analyzer-null-dereference.

[wtarreau]

It seems that the double-free check isn't any better:

src/namespace.c: In function 'netns_sig_stop':
src/namespace.c:55:17: error: double-'free' of '*(struct netns_entry *)node.node.key' [CWE-415] [-Werror=analyzer-double-fre]
   55 |                 free(entry->node.key);
      |                 ^~~~~~~~~~~~~~~~~~~~~

When you look at the code, it's a free() of a single element that's inside a loop that iterates over a tree. So you can happily add -Wno-analyzer-double-free too.

[shipitsin-ilia]

I recall we reported some false positives for gcc11, I'll check with gcc master

[wtarreau]

Oh and the cherry on the cake:

In function 'ssl_parse_global_ciphers':
src/cfgparse-ssl.c:264:17: error: leak of 'strdup(args[1])' [CWE-401] [-Werror=analyzer-malloc-leak]
  264 |         *target = strdup(args[1]);
      |         ~~~~~~~~^~~~~~~~~~~~~~~~~

How to explain... by definition if it's stored where it ought to be used, it's not leaked. I think this analyzer has serious issues with data lifecycle and is totally unusable. Most likely it's still in its infancy and will start to be usable in 10 years from now on hello world-like programs but here we must not waste our time and energy on this one. At the very least you can also add -Wno-analyzer-malloc-leak.

[wtarreau]

Here it's not just some false positives, it's a totally bogus analysis. The concepts themselves are totally flawed. Maybe try with clang, to see if it does something less ridiculous ?

[wtarreau]

The analyzer-use-after-free analyzer is wrong as well, just like the double-free one, it doesn't understand loops and believes that the second call of a function to free an item applies to the same as the previous one instead of the next item in the list.

[chipitsine]

clang is in my queue as well

…

On Thu, Dec 29, 2022, 4:08 PM Willy Tarreau ***@***.***> wrote: Here it's not just some false positives, it's a totally bogus analysis. The concepts themselves are totally flawed. Maybe try with clang, to see if it does something less ridiculous ? — Reply to this email directly, view it on GitHub <#1745 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ5KUDH5LHKOKU6J5X4UVTWPVPIXANCNFSM5YP33FDQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[wtarreau]

And this very long one is wrong as well:

include/import/ist.h: In function 'parse_tcpcheck_expect':
src/tcpcheck.c:3336:59: warning: use of NULL 'pattern' where non-null expected [CWE-476] [-Wanalyzer-null-argument]
 3336 |                         c1 = c2 = read_uint(&p, pattern + strlen(pattern));
      |                                                           ^~~~~~~~~~~~~~~

The value is used when type==TCPCHK_EXPECT_HTTP_STATUS which is only set when pattern is assigned a non-null value. In short it's 100% wrong for now, with reports that are extremely difficult to handle because we start from the assumption that this thing knows C and can follow loops and conditions, which is not true.

[wtarreau]

I'm giving up now. I've wasted two hours on this garbage, that's not productive use of our time :-( It seems that the only one that seldom reports true problems is -Wanalyzer-possible-null-dereference. It catches unchecked malloc/calloc at boot. These are mostly harmless but it's always better to address them. The other ones are far too bogus to even spend any more time just reading their title.

[chipitsine]

Thank you for the review, I'll check on master branch and report them to GCC. Hopefully, if we will contact them once an year, it will be working in 10 years))

…

On Thu, Dec 29, 2022, 4:42 PM Willy Tarreau ***@***.***> wrote: And this very long one is wrong as well: include/import/ist.h: In function 'parse_tcpcheck_expect': src/tcpcheck.c:3336:59: warning: use of NULL 'pattern' where non-null expected [CWE-476] [-Wanalyzer-null-argument] 3336 | c1 = c2 = read_uint(&p, pattern + strlen(pattern)); | ^~~~~~~~~~~~~~~ The value is used when type==TCPCHK_EXPECT_HTTP_STATUS which is only set when pattern is assigned a non-null value. In short it's 100% wrong for now, with reports that are extremely difficult to handle because we start from the assumption that this thing knows C and can follow loops and conditions, which is not true. — Reply to this email directly, view it on GitHub <#1745 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ5KUDY3HT4E6FLQJDVGELWPVTJ3ANCNFSM5YP33FDQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[wtarreau]

I don't think it's worth annoying them with that. I suspect the analyzer might be a side project for them (probably for students since it does involve some language theory, control flow integrity etc) and it's very likely that the paint is not dry yet. I'd rather make sure they spend their time addressing regular annoying warnings than this monster. Such tools are not always accurate and we just need to use what appears to provide value. We already have Coverity that reports real concerns (thanks to your filtering). Maybe clang will do as well. Do we need yet another one ? Probably not, especially if it starts on such unstable grounds.

[chipitsine]

It seems that the double-free check isn't any better:

src/namespace.c: In function 'netns_sig_stop':
src/namespace.c:55:17: error: double-'free' of '*(struct netns_entry *)node.node.key' [CWE-415] [-Werror=analyzer-double-fre]
   55 |                 free(entry->node.key);
      |                 ^~~~~~~~~~~~~~~~~~~~~

When you look at the code, it's a free() of a single element that's inside a loop that iterates over a tree. So you can happily add -Wno-analyzer-double-free too.

this one is fixed in gcc master

[chipitsine]

couple of issues reported to gcc

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108251
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108252

closing this issue

[shipitsin-ilia]

@wtarreau , have a look :)

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108252
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=688fc162b76dc6747a30fcfd470f4770da0f4924

tadam!

[wtarreau]

OK well done! Just make sure to try to file issues in a reproducible enough way so that they don't spend too much time trying to isolate them. This time it looks like it worked well though.

[shipitsin-ilia]

both false positive get attention from RedHat, I beleive that RedHat is interested in adding -fanalyzer to every RPM build (which is easy by adding CFLAGS)