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

public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed

* [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
@ 2021-10-11 21:47 eggert at cs dot ucla.edu
  2021-10-11 22:21 ` [Bug analyzer/102692] " eggert at cs dot ucla.edu
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: eggert at cs dot ucla.edu @ 2021-10-11 21:47 UTC (permalink / raw)
  To: gcc-bugs

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
  2021-10-11 21:47 [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next) eggert at cs dot ucla.edu
@ 2021-10-11 22:21 ` eggert at cs dot ucla.edu
  2022-01-07 22:46 ` dmalcolm at gcc dot gnu.org
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: eggert at cs dot ucla.edu @ 2021-10-11 22:21 UTC (permalink / raw)
  To: gcc-bugs

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...
    |

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
  2021-10-11 21:47 [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next) eggert at cs dot ucla.edu
  2021-10-11 22:21 ` [Bug analyzer/102692] " eggert at cs dot ucla.edu
@ 2022-01-07 22:46 ` dmalcolm at gcc dot gnu.org
  2022-01-11 14:17 ` cvs-commit at gcc dot gnu.org
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2022-01-07 22:46 UTC (permalink / raw)
  To: gcc-bugs

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
  2021-10-11 21:47 [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next) eggert at cs dot ucla.edu
  2021-10-11 22:21 ` [Bug analyzer/102692] " eggert at cs dot ucla.edu
  2022-01-07 22:46 ` dmalcolm at gcc dot gnu.org
@ 2022-01-11 14:17 ` cvs-commit at gcc dot gnu.org
  2022-02-11 17:40 ` dmalcolm at gcc dot gnu.org
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-01-11 14:17 UTC (permalink / raw)
  To: gcc-bugs

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
  2021-10-11 21:47 [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next) eggert at cs dot ucla.edu
                   ` (2 preceding siblings ...)
  2022-01-11 14:17 ` cvs-commit at gcc dot gnu.org
@ 2022-02-11 17:40 ` dmalcolm at gcc dot gnu.org
  2022-02-15 21:34 ` cvs-commit at gcc dot gnu.org
  2022-02-15 21:39 ` dmalcolm at gcc dot gnu.org
  5 siblings, 0 replies; 7+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2022-02-11 17:40 UTC (permalink / raw)
  To: gcc-bugs

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
  2021-10-11 21:47 [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next) eggert at cs dot ucla.edu
                   ` (3 preceding siblings ...)
  2022-02-11 17:40 ` dmalcolm at gcc dot gnu.org
@ 2022-02-15 21:34 ` cvs-commit at gcc dot gnu.org
  2022-02-15 21:39 ` dmalcolm at gcc dot gnu.org
  5 siblings, 0 replies; 7+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-02-15 21:34 UTC (permalink / raw)
  To: gcc-bugs

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug analyzer/102692] -Wanalyzer-null-dereference false alarm with (!p || q || !p->next)
  2021-10-11 21:47 [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next) eggert at cs dot ucla.edu
                   ` (4 preceding siblings ...)
  2022-02-15 21:34 ` cvs-commit at gcc dot gnu.org
@ 2022-02-15 21:39 ` dmalcolm at gcc dot gnu.org
  5 siblings, 0 replies; 7+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2022-02-15 21:39 UTC (permalink / raw)
  To: gcc-bugs

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-02-15 21:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11 21:47 [Bug analyzer/102692] New: -Wanalyzer-null-dereference false alarm with (!p || q || !p->next) eggert at cs dot ucla.edu
2021-10-11 22:21 ` [Bug analyzer/102692] " eggert at cs dot ucla.edu
2022-01-07 22:46 ` dmalcolm at gcc dot gnu.org
2022-01-11 14:17 ` cvs-commit at gcc dot gnu.org
2022-02-11 17:40 ` dmalcolm at gcc dot gnu.org
2022-02-15 21:34 ` cvs-commit at gcc dot gnu.org
2022-02-15 21:39 ` dmalcolm at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).