Bug 101962 - Analyzer NULL false positive with pointer manipulation
Summary: Analyzer NULL false positive with pointer manipulation
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: David Malcolm
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2021-08-18 16:40 UTC by David Malcolm
Modified: 2022-11-08 22:50 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-08-18 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Malcolm 2021-08-18 16:40:33 UTC
-fanalyzer emits two warnings on this code:

#define NULL ((void *)0)

int *
func1(int *ptr) {
  if (!ptr)
    return NULL;
  return ++ptr;
}

int
main() {
  int stack;
  int *a = &stack;
  a = func1(a);
  a = func1(a);
  return *a;
}

Compiler Explorer link: https://godbolt.org/z/ohecfvdd8

gcc 11.2 emits:
  <source>:16:10: warning: dereference of NULL 'a' [CWE-476] [-Wanalyzer-null-dereference]
     16 |   return *a;
        |          ^~
for the path in which ptr is non-NULL in the first call, and then NULL in the 2nd call, i.e. for which &stack == (NULL) - 1.

Whilst this is technically correct, it won't occur in practise and is thus effectively a false positive that we shouldn't warn for.

trunk also emits:
  <source>:16:10: warning: use of uninitialized value '*a' [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
     16 |   return *a;
        |          ^~
for the path in which ptr is non-NULL in both calls, and so we're effectively accessing (&stack)[2], which is a true problem in the software under test, but would be better to report as an out-of-bounds warning (the analyzer doesn't yet do bounds checking).

Downstream report: https://bugzilla.redhat.com/show_bug.cgi?id=1995092
Comment 1 David Malcolm 2021-08-18 21:22:35 UTC
Am testing a fix.
Comment 2 GCC Commits 2021-08-23 18:08:49 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:e82e0f149b0aba660896ea9aa12c442c07a16d12

commit r12-3094-ge82e0f149b0aba660896ea9aa12c442c07a16d12
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Mon Aug 23 14:07:39 2021 -0400

    analyzer: assume that POINTER_PLUS_EXPR of non-NULL is non-NULL [PR101962]
    
    gcc/analyzer/ChangeLog:
            PR analyzer/101962
            * region-model.cc (region_model::eval_condition_without_cm):
            Refactor comparison against zero, adding a check for
            POINTER_PLUS_EXPR of non-NULL.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/101962
            * gcc.dg/analyzer/data-model-23.c: New test.
            * gcc.dg/analyzer/pr101962.c: New test.
Comment 3 David Malcolm 2021-08-23 18:26:40 UTC
Should be fixed on trunk for gcc 12 by the above commit.

I plan to backport this to gcc 11; keeping it open until that's done.
Comment 4 Marek Polacek 2021-12-07 20:24:03 UTC
Any update on the backport?
Comment 5 GCC Commits 2021-12-11 02:56:00 UTC
The releases/gcc-11 branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:de0656f98640a57cd9dfdb090264afaa06ba46cc

commit r11-9374-gde0656f98640a57cd9dfdb090264afaa06ba46cc
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Mon Aug 23 14:07:39 2021 -0400

    analyzer: assume that POINTER_PLUS_EXPR of non-NULL is non-NULL [PR101962]
    
    Backported from commit r12-3094-ge82e0f149b0aba660896ea9aa12c442c07a16d12,
    dropping the expected "use of uninitialized value" warning from
    pr101962.c
    
    gcc/analyzer/ChangeLog:
            PR analyzer/101962
            * region-model.cc (region_model::eval_condition_without_cm):
            Refactor comparison against zero, adding a check for
            POINTER_PLUS_EXPR of non-NULL.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/101962
            * gcc.dg/analyzer/data-model-23.c: New test.
            * gcc.dg/analyzer/pr101962.c: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Comment 6 David Malcolm 2021-12-11 02:57:06 UTC
Backported to gcc 11 by the above commit.  I don't plan to backport to gcc 10; marking this as resolved.
Comment 7 GCC Commits 2022-11-08 22:50:15 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:9bbcee450deb0f561b096924a3f148369333e54c

commit r13-3819-g9bbcee450deb0f561b096924a3f148369333e54c
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Tue Nov 8 17:49:07 2022 -0500

    analyzer: eliminate region_model::eval_condition_without_cm [PR101962]
    
    In r12-3094-ge82e0f149b0aba I added the assumption that
    POINTER_PLUS_EXPR of non-NULL is non-NULL (for PR analyzer/101962).
    
    Whilst working on another bug, I noticed that this only works
    when the LHS is known to be non-NULL via
    region_model::eval_condition_without_cm, but not when it's known through
    a constraint.
    
    This distinction predates the original commit of the analyzer in GCC 10,
    but I believe it became irrelevant in the GCC 11 rewrite of the region
    model code (r11-2694-g808f4dfeb3a95f).
    
    Hence this patch eliminates region_model::eval_condition_without_cm in
    favor of all users simply calling region_model::eval_condition.  Doing
    so enables the "POINTER_PLUS_EXPR of non-NULL is non-NULL" assumption to
    also be made when the LHS is known through a constraint (e.g. a
    conditional).
    
    gcc/analyzer/ChangeLog:
            PR analyzer/101962
            * region-model-impl-calls.cc: Update comment.
            * region-model.cc (region_model::check_symbolic_bounds): Fix
            layout of "void" return.  Replace usage of
            eval_condition_without_cm with eval_condition.
            (region_model::eval_condition): Take over body of...
            (region_model::eval_condition_without_cm): ...this subroutine,
            dropping the latter.  Eliminating this distinction avoids issues
            where constraints were not considered when recursing.
            (region_model::compare_initial_and_pointer): Update comment.
            (region_model::symbolic_greater_than): Replace usage of
            eval_condition_without_cm with eval_condition.
            * region-model.h
            (region_model::eval_condition_without_cm): Delete decl.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/101962
            * gcc.dg/analyzer/data-model-23.c (test_3): New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>