Bug 102692 - -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
Summary: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 11.2.1
: P3 normal
Target Milestone: ---
Assignee: David Malcolm
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2021-10-11 21:47 UTC by eggert
Modified: 2022-02-15 21:39 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-01-07 00:00:00


Attachments
compile on x86-64 with "gcc -fanalyzer -O2 -S analyzer-null-dereference-simple.i" (754 bytes, text/plain)
2021-10-11 21:47 UTC, eggert
Details

Note You need to log in before you can comment on or make changes to this bug.
Description eggert 2021-10-11 21:47:53 UTC
Created attachment 51588 [details]
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.
Comment 1 eggert 2021-10-11 22:21:58 UTC
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...
    |
Comment 2 David Malcolm 2022-01-07 22:46:03 UTC
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.
Comment 3 CVS Commits 2022-01-11 14:17:26 UTC
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>
Comment 4 David Malcolm 2022-02-11 17:40:44 UTC
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.
Comment 5 CVS Commits 2022-02-15 21:34:11 UTC
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>
Comment 6 David Malcolm 2022-02-15 21:39:04 UTC
Should be fixed (or, at least, worked around) by the above patch; marking as resolved.