[Bug analyzer/108251] New: false positive: null dereference

[Bug analyzer/108251] New: false positive: null dereference

[chipitsine at gmail dot com]

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

            Bug ID: 108251
           Summary: false positive: null dereference
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: chipitsine at gmail dot com
  Target Milestone: ---

repro steps:

git clone https://github.com/haproxy/haproxy
cd haproxy
export CC=/home/ilia/gcc/gcc-home/bin/gcc 
make TARGET=linux-glibc USE_OPENSSL=1 DEBUG_CFLAGS="-fanalyzer"

this finding is wrong

```
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>'
    |
```

if conn is null, we'll return here (two lines above):

```
        ssl = ssl_sock_get_ssl_object(conn);
        if (!ssl)
                return 0;
```

detailed review:
https://github.com/haproxy/haproxy/issues/1745#issuecomment-1367200781

[Bug analyzer/108251] false positive: null dereference

[dmalcolm at gcc dot gnu.org]

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

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Created attachment 54219
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54219&action=edit
Simplified reproducer for smp_fetch_ssl_fc_has_early

Thanks for filing this bug.  I see the warnings, and have reduced the
smp_fetch_ssl_fc_has_early example to the attached.

[Bug analyzer/108251] false positive: null dereference

[dmalcolm at gcc dot gnu.org]

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

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
With trunk and -Wno-address-of-packed-member -O2, I get:

../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c: In function
‘smp_fetch_ssl_fc_has_early’:
../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:92:27: warning:
dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
      |                       ~~~~^~~~~~~
  ‘smp_fetch_ssl_fc_has_early’: events 1-3
    |
    |   85 |  if (!ssl)
    |      |     ^
    |      |     |
    |      |     (1) following ‘false’ branch (when ‘ssl’ is non-NULL)...
    |......
    |   88 |  smp->flags = 0;
    |      |  ~~~~~~~~~~~~~~
    |      |             |
    |      |             (2) ...to here
    |......
    |   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
    |      |                       ~~~~~~~~~~~
    |      |                           |
    |      |                           (3) dereference of NULL ‘<unknown>’
    |

[Bug analyzer/108251] false positive: null dereference

[dmalcolm at gcc dot gnu.org]

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

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Adding  -fanalyzer-verbosity=3 to comment #2, I get:

../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c: In function
‘smp_fetch_ssl_fc_has_early’:
../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:92:27: warning:
dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
      |                       ~~~~^~~~~~~
  ‘smp_fetch_ssl_fc_has_early’: events 1-2
    |
    |   78 | smp_fetch_ssl_fc_has_early(const struct arg *args, struct sample
*smp, const char *kw, void *private)
    |      | ^~~~~~~~~~~~~~~~~~~~~~~~~~
    |      | |
    |      | (1) entry to ‘smp_fetch_ssl_fc_has_early’
    |......
    |   83 |  conn = objt_conn(smp->sess->origin);
    |      |         ~
    |      |         |
    |      |         (2) inlined call to ‘objt_conn’ from
‘smp_fetch_ssl_fc_has_early’
    |
    +--> ‘objt_conn’: event 3
           |
           |   63 |  if (!t || *t != OBJ_TYPE_CONN)
           |      |     ^
           |      |     |
           |      |     (3) following ‘true’ branch...
           |
    <------+
    |
  ‘smp_fetch_ssl_fc_has_early’: event 4
    |
    |cc1:
    | (4): ...to here
    |
  ‘smp_fetch_ssl_fc_has_early’: events 5-7
    |
    |   85 |  if (!ssl)
    |      |     ^
    |      |     |
    |      |     (5) following ‘false’ branch (when ‘ssl’ is non-NULL)...
    |......
    |   88 |  smp->flags = 0;
    |      |  ~~~~~~~~~~~~~~
    |      |             |
    |      |             (6) ...to here
    |......
    |   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
    |      |                       ~~~~~~~~~~~
    |      |                           |
    |      |                           (7) dereference of NULL ‘<unknown>’
    |

which is slightly clearer; arguably we shouldn't have pruned events 2-4 from
this at the lower verbosity level, since it's really hard to figure out what
the analyzer is "thinking" in comment #2.

[Bug analyzer/108251] false positive: null dereference

[dmalcolm at gcc dot gnu.org]

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

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Without optimization, trunk with just -Wno-address-of-packed-member (and
-fanalyzer), I get:

../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c: In function
‘smp_fetch_ssl_fc_has_early’:
../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:92:27: warning:
dereference of NULL ‘conn’ [CWE-476] [-Wanalyzer-null-dereference]
   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
      |                       ~~~~^~~~~~~
  ‘smp_fetch_ssl_fc_has_early’: events 1-2
    |
    |   78 | smp_fetch_ssl_fc_has_early(const struct arg *args, struct sample
*smp, const char *kw, void *private)
    |      | ^~~~~~~~~~~~~~~~~~~~~~~~~~
    |      | |
    |      | (1) entry to ‘smp_fetch_ssl_fc_has_early’
    |......
    |   83 |  conn = objt_conn(smp->sess->origin);
    |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (2) calling ‘objt_conn’ from ‘smp_fetch_ssl_fc_has_early’
    |
    +--> ‘objt_conn’: events 3-4
           |
           |   61 | static inline struct connection *objt_conn(enum obj_type
*t)
           |      |                                  ^~~~~~~~~
           |      |                                  |
           |      |                                  (3) entry to ‘objt_conn’
           |......
           |   64 |    return ((void *)0);
           |      |           ~                       
           |      |           |
           |      |           (4) ‘0’ is NULL
           |
    <------+
    |
  ‘smp_fetch_ssl_fc_has_early’: events 5-8
    |
    |   83 |  conn = objt_conn(smp->sess->origin);
    |      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (5) return of NULL to ‘smp_fetch_ssl_fc_has_early’ from
‘objt_conn’
    |   84 |  ssl = ssl_sock_get_ssl_object(conn);
    |   85 |  if (!ssl)
    |      |     ~    
    |      |     |
    |      |     (6) following ‘false’ branch (when ‘ssl’ is non-NULL)...
    |......
    |   88 |  smp->flags = 0;
    |      |  ~~~~~~~~~~~~~~
    |      |             |
    |      |             (7) ...to here
    |......
    |   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
    |      |                       ~~~~~~~~~~~
    |      |                           |
    |      |                           (8) dereference of NULL ‘conn’
    |

[Bug analyzer/108251] false positive: null dereference

[dmalcolm at gcc dot gnu.org]

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

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
As per comment #4 (optimization disabled), but adding: -fanalyzer-verbosity=3
makes things clearer:

../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c: In function
‘smp_fetch_ssl_fc_has_early’:
../../src/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:92:27: warning:
dereference of NULL ‘conn’ [CWE-476] [-Wanalyzer-null-dereference]
   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
      |                       ~~~~^~~~~~~
  ‘smp_fetch_ssl_fc_has_early’: events 1-2
    |
    |   78 | smp_fetch_ssl_fc_has_early(const struct arg *args, struct sample
*smp, const char *kw, void *private)
    |      | ^~~~~~~~~~~~~~~~~~~~~~~~~~
    |      | |
    |      | (1) entry to ‘smp_fetch_ssl_fc_has_early’
    |......
    |   83 |  conn = objt_conn(smp->sess->origin);
    |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (2) calling ‘objt_conn’ from ‘smp_fetch_ssl_fc_has_early’
    |
    +--> ‘objt_conn’: events 3-6
           |
           |   61 | static inline struct connection *objt_conn(enum obj_type
*t)
           |      |                                  ^~~~~~~~~
           |      |                                  |
           |      |                                  (3) entry to ‘objt_conn’
           |   62 | {
           |   63 |  if (!t || *t != OBJ_TYPE_CONN)
           |      |     ~                             
           |      |     |
           |      |     (4) following ‘true’ branch (when ‘t’ is NULL)...
           |   64 |    return ((void *)0);
           |      |           ~                       
           |      |           |
           |      |           (5) ...to here
           |      |           (6) ‘0’ is NULL
           |
    <------+
    |
  ‘smp_fetch_ssl_fc_has_early’: events 7-10
    |
    |   83 |  conn = objt_conn(smp->sess->origin);
    |      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (7) return of NULL to ‘smp_fetch_ssl_fc_has_early’ from
‘objt_conn’
    |   84 |  ssl = ssl_sock_get_ssl_object(conn);
    |   85 |  if (!ssl)
    |      |     ~    
    |      |     |
    |      |     (8) following ‘false’ branch (when ‘ssl’ is non-NULL)...
    |......
    |   88 |  smp->flags = 0;
    |      |  ~~~~~~~~~~~~~~
    |      |             |
    |      |             (9) ...to here
    |......
    |   92 |  smp->data.u.sint = ((conn->flags & CO_FL_EARLY_DATA) &&
    |      |                       ~~~~~~~~~~~
    |      |                           |
    |      |                           (10) dereference of NULL ‘conn’
    |

[Bug analyzer/108251] false positive: null dereference

[dmalcolm at gcc dot gnu.org]

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

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
The analyzer sees the error-handling case in objt_conn, and considers the
execution path where it bails out early due to "t" being NULL i.e.
smp->sess->origin is NULL, and thus conn being initialized to NULL.

At it turns out, ssl_sock_get_ssl_object is defined (in src/ssl_sock.c) as:

SSL *ssl_sock_get_ssl_object(struct connection *conn)
{
        struct ssl_sock_ctx *ctx = conn_get_ssl_sock_ctx(conn);

        return ctx ? ctx->ssl : NULL;
}

and conn_get_ssl_sock_ctx is defined (in include/haproxy/connection.h) as:

/* retrieves the ssl_sock_ctx for this connection otherwise NULL */
static inline struct ssl_sock_ctx *conn_get_ssl_sock_ctx(struct connection
*conn)
{
        if (!conn || !conn->xprt || !conn->xprt->get_ssl_sock_ctx)
                return NULL;
        return conn->xprt->get_ssl_sock_ctx(conn);
}

Hence it's a false positive: if conn is NULL, then ssl_sock_get_ssl_object will
return NULL.

The TU "sees" the definition of conn_get_ssl_sock_ctx, but only the declaration
of ssl_sock_get_ssl_object, not the latter's declaration.

Without a definition of ssl_sock_get_ssl_object, -fanalyzer can't "know" of the
interaction between "ssl" and "conn" (-fanalyzer doesn't work well with LTO
yet).

Hence it erroneously considers the case that ssl_sock_get_ssl_object could
return non-NULL on a NULL "conn", and sees the deref of conn, which it reports.

[Bug analyzer/108251] -Wanalyzer-null-dereference false positive seen in haproxy's src/ssl_sample.c

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:4214bdb1d77ebee04d12f66c831730ed67fedf55

commit r13-6565-g4214bdb1d77ebee04d12f66c831730ed67fedf55
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Mar 9 16:21:02 2023 -0500

    testsuite: add various -Wanalyzer-null-dereference false +ve test cases

    There are various -Wanalyzer-null-dereference false +ves in bugzilla
    that I've been attempting to fix.  Unfortunately I haven't made much
    progress, but it seems worth at least capturing the reduced
    reproducers as test cases, to make it easier to spot changes in
    behavior.

    gcc/testsuite/ChangeLog:
            PR analyzer/102671
            PR analyzer/105755
            PR analyzer/108251
            PR analyzer/108400
            * gcc.dg/analyzer/null-deref-pr102671-1.c: New test, reduced
            from Emacs.
            * gcc.dg/analyzer/null-deref-pr102671-2.c: Likewise.
            * gcc.dg/analyzer/null-deref-pr105755.c: Likewise.
            *
gcc.dg/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c:
            New test, reduced from haproxy's src/ssl_sample.c.
            * gcc.dg/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:
            Likewise.
            * gcc.dg/analyzer/null-deref-pr108400-SoftEtherVPN-WebUi.c: New
            test, reduced from SoftEtherVPN's src/Cedar/WebUI.c.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>