[Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[eggert at cs dot ucla.edu]

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

            Bug ID: 102692
           Summary: -Wanalyzer-null-dereference false alarm with (!p || q
                    || !p->next)
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: eggert at cs dot ucla.edu
  Target Milestone: ---

Created attachment 51588
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51588&action=edit
compile on x86-64 with "gcc -fanalyzer -O2 -S
analyzer-null-dereference-simple.i"

I ran into this problem when compiling GNU Emacs with gcc 11.2.1 20210728 (Red
Hat 11.2.1-1) on x86-64. Compile the attached simplified version with:

gcc -fanalyzer -O2 -S analyzer-null-dereference-simple.i

and the output will be as at the end of this description. This output is bogus
since it complains that 'tail' might be null in (!tail || end < prev ||
!tail->next) which means that tail->next might dereference a null pointer. But
the tail->next expression is obviously unreachable if 'tail' is null.

Although this bug might be related to GCC bug 102671, I'm filing it separately
as it has a different feel.

[Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[eggert at cs dot ucla.edu]

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

--- Comment #1 from eggert at cs dot ucla.edu ---
Sorry, forgot to mention the incorrect GCC output. Here it is:
-----
analyzer-null-dereference-simple.i: In function ‘fix_overlays_before’:
analyzer-null-dereference-simple.i:79:35: warning: dereference of NULL ‘tail’
[CWE-476] [-Wanalyzer-null-dereference]
   79 |   if (!tail || end < prev || !tail->next)
      |                               ~~~~^~~~~~
  ‘fix_overlays_before’: events 1-5
    |
    |   72 |   while (tail
    |      |          ~~~~
    |   73 |          && (tem = make_lisp_ptr (tail, 5),
    |      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |          |
    |      |          (1) following ‘false’ branch (when ‘tail’ is NULL)...
    |   74 |              (end = marker_position (XOVERLAY (tem)->end)) >=
pos))
    |      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |......
    |   79 |   if (!tail || end < prev || !tail->next)
    |      |      ~~~~~~                   ~~~~~~~~~~
    |      |      ||                           |
    |      |      ||                           (4) ...to here
    |      |      |(2) ...to here              (5) dereference of NULL ‘tail’
    |      |      (3) following ‘false’ branch...
    |

[Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-01-07
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for filing this.  With trunk (for gcc 12) I see the false positive from
comment #1, and also this false positive (new warning in gcc 12):
../../src/gcc/testsuite/gcc.dg/analyzer/pr102692.c:82:20: warning: use of
uninitialized value ‘end’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
   82 |   if (!tail || end < prev || !tail->next) /* { dg-bogus "use of
uninitialized value 'end'" "uninit" } */
      |                ~~~~^~~~~~
  ‘fix_overlays_before’: events 1-3
    |
    |   75 |   while (tail
    |      |          ~~~~
    |   76 |          && (tem = make_lisp_ptr (tail, 5),
    |      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |          |
    |      |          (1) following ‘false’ branch (when ‘tail’ is NULL)...
    |   77 |              (end = marker_position (XOVERLAY (tem)->end)) >=
pos))
    |      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |......
    |   82 |   if (!tail || end < prev || !tail->next) /* { dg-bogus "use of
uninitialized value 'end'" "uninit" } */
    |      |       ~~~~~    ~~~~~~~~~~
    |      |       |            |
    |      |       |            (3) use of uninitialized value ‘end’ here
    |      |       (2) ...to here
    |

Am investigating.

[Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 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:4f34f8cc1d064bfaaed723312c71e92f495d429b

commit r12-6476-g4f34f8cc1d064bfaaed723312c71e92f495d429b
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Jan 7 17:44:23 2022 -0500

    analyzer: fix false +ve on bitwise binops (PR analyzer/102692)

    PR analyzer/102692 reports a false positive at -O2 from
    -Wanalyzer-null-dereference on:
      if (!p || q || !p->next)

    At the gimple level, -O2 has converted the first || into bitwise or
    controlling a jump:
      _4 = _2 | _3;
      if (_4 != 0)
    and a recursive call has been converted to iteration.  The analyzer hits
    the symbolic value complexity limit whilst analyzing the iteration and
    hits a case where _2 is (_Bool)1 (i.e. true) and _3 (i.e. q) is
    UNKNOWN(_Bool).

    There are two issues leading to the false positive; fixing either issue
    fixes the false positive; this patch fixes both for good measure:

    (a) The analyzer erronously treats bitwise ops on UNKNOWN(_Bool) as
UNKNOWN,
    even for case like (1 | UNKNOWN) where we know the result, leading to
    bogus edges in the exploded graph.  The patch fixes these cases.

    (b) The state-handling code creates "UNKNOWN" symbolic values, as a way
    to give up when symbolic expressions get too complicated, and in various
    other cases.  This makes sense when building the exploded graph, since
    we want the analysis to terminate, but doesn't make sense when checking
    the feasibility along a specific path, where we want precision.  The patch
    prevents all use of "unknown" symbolic values when performing feasibility
    checking of a path (before it merely stopped doing complexity-checking),
    by creating a unique placeholder_svalue instead.

    This fixes the -Wanalyzer-null-dereference false positive.

    Unfortunately, with GCC 12 there's also a false positive from
    -Wanalyzer-use-of-uninitialized-value on this code, which is a separate
    issue that the patch does not fix.

    gcc/analyzer/ChangeLog:
            PR analyzer/102692
            * diagnostic-manager.cc
            (class auto_disable_complexity_checks): Rename to...
            (class auto_checking_feasibility): ...this, updating
            the calls accordingly.
            (epath_finder::explore_feasible_paths): Update for renaming.
            * region-model-manager.cc
            (region_model_manager::region_model_manager): Update for change
from
            m_check_complexity to m_checking_feasibility.
            (region_model_manager::reject_if_too_complex): Likewise.
            (region_model_manager::get_or_create_unknown_svalue): Handle
            m_checking_feasibility.
            (region_model_manager::create_unique_svalue): New.
            (region_model_manager::maybe_fold_binop): Handle BIT_AND_EXPR and
            BIT_IOR_EXPRs on booleans where we know the result.
            * region-model.cc (test_binop_svalue_folding): Add test coverage
            for the above.
            * region-model.h (region_model_manager::create_unique_svalue): New
            decl.
            (region_model_manager::enable_complexity_check): Replace with...
            (region_model_manager::begin_checking_feasibility): ...this.
            (region_model_manager::disable_complexity_check): Replace with...
            (region_model_manager::end_checking_feasibility): ...this.
            (region_model_manager::m_check_complexity): Replace with...
            (region_model_manager::m_checking_feasibility): ...this.
            (region_model_manager::m_managed_dynamic_svalues): New field.

    gcc/testsuite/ChangeLog:
            PR analyzer/102692
            * gcc.dg/analyzer/pr102692.c: New test.

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

[Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[dmalcolm at gcc dot gnu.org]

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

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
I've been investigating the false positive from
-Wanalyzer-use-of-uninitialized-value.  It only happens when optimization is
turned on, but happens within the FE, before gimplification.

Specifically, c_parser_condition parses this condition:
  ((A OR-IF B) OR-IF C)
and calls c_fully_fold on the condition.

Within c_fully_fold, fold_truth_andor is called, which bails when optimization
is off, but if any optimization is turned on converts the
  ((A OR-IF B) OR-IF C)
into:
  ((A OR B) OR_IF C)

and at gimplification time the inner OR becomes BIT_IOR_EXPR (in
gimplify_expr), giving this for the inner condition:

  tmp = A | B
  if (tmp)

thus effectively synthesizing a redundant access of B when optimization is
turned on.

[Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 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:1e2fe6715a949f80c1204ae244baad3cd80ffaf0

commit r12-7251-g1e2fe6715a949f80c1204ae244baad3cd80ffaf0
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Feb 11 16:43:21 2022 -0500

    analyzer: fix uninit false +ve due to optimized conditionals [PR102692]

    There is false positive from -Wanalyzer-use-of-uninitialized-value on
    gcc.dg/analyzer/pr102692.c here:

      âfix_overlays_beforeâ: events 1-3
        |
        |   75 |   while (tail
        |      |          ~~~~
        |   76 |          && (tem = make_lisp_ptr (tail, 5),
        |      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        |      |          |
        |      |          (1) following âfalseâ branch (when âtailâ is
NULL)...
        |   77 |              (end = marker_position (XOVERLAY (tem)->end)) >=
pos))
        |      |             
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        |......
        |   82 |   if (!tail || end < prev || !tail->next)
        |      |       ~~~~~    ~~~~~~~~~~
        |      |       |            |
        |      |       |            (3) use of uninitialized value âendâ
here
        |      |       (2) ...to here
        |

    The issue is that inner || of the conditionals have been folded within the
    frontend from a chain of control flow:

       5   â   if (tail == 0B) goto <D.1986>; else goto <D.1988>;
       6   â   <D.1988>:
       7   â   if (end < prev) goto <D.1986>; else goto <D.1989>;
       8   â   <D.1989>:
       9   â   _1 = tail->next;
      10   â   if (_1 == 0B) goto <D.1986>; else goto <D.1987>;
      11   â   <D.1986>:

    to an OR expr (and then to a bitwise-or by the gimplifier):

       5   â   _1 = tail == 0B;
       6   â   _2 = end < prev;
       7   â   _3 = _1 | _2;
       8   â   if (_3 != 0) goto <D.1986>; else goto <D.1988>;
       9   â   <D.1988>:
      10   â   _4 = tail->next;
      11   â   if (_4 == 0B) goto <D.1986>; else goto <D.1987>;

    This happens for sufficiently simple conditionals in fold_truth_andor.
    In particular, the (end < prev) is short-circuited without optimization,
    but is evaluated with optimization, leading to the false positive.

    Given how early this folding occurs, it seems the simplest fix is to
    try to detect places where this optimization appears to have happened,
    and suppress uninit warnings within the statement that would have
    been short-circuited.

    gcc/analyzer/ChangeLog:
            PR analyzer/102692
            * exploded-graph.h (impl_region_model_context::get_stmt): New.
            * region-model.cc: Include "gimple-ssa.h", "tree-phinodes.h",
            "tree-ssa-operands.h", and "ssa-iterators.h".
            (within_short_circuited_stmt_p): New.
            (region_model::check_for_poison): Don't warn about uninit values
            if within_short_circuited_stmt_p.
            * region-model.h (region_model_context::get_stmt): New vfunc.
            (noop_region_model_context::get_stmt): New.

    gcc/testsuite/ChangeLog:
            PR analyzer/102692
            * gcc.dg/analyzer/pr102692-2.c: New test.
            * gcc.dg/analyzer/pr102692.c: Remove xfail.  Remove -O2 from
            options and move to...
            * gcc.dg/analyzer/torture/pr102692.c: ...here.

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

[Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed (or, at least, worked around) by the above patch; marking as
resolved.