[Bug analyzer/94362] New: False analyzer report due to i >= 0 and i < 0 on openssl

[Bug analyzer/94362] New: False analyzer report due to i >= 0 and i < 0 on openssl

[dmalcolm at gcc dot gnu.org]

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

            Bug ID: 94362
           Summary: False analyzer report due to i >= 0 and i < 0 on
                    openssl
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: dmalcolm at gcc dot gnu.org
  Target Milestone: ---

Created attachment 48134
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48134&action=edit
Reduced test case

https://github.com/openssl/openssl/issues/11420 reports what looks like a false
positive:
  crypto/asn1/ameth_lib.c:131:18: error: dereference of NULL 'ameth' [CWE-690]
[-Werror=analyzer-null-dereference]

where on the path to the diagnostic i >= 0 and i < 0, which ought to be
rejected by constraint-checking.

I'm attaching a somewhat simplified reproducer.

[Bug analyzer/94362] False analyzer report due to i >= 0 and i < 0 on openssl

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-02-17
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Looks like this no longer affects trunk.

I can reproduce the false positive with -fno-analyzer-feasibility, but by
default, the diagnostic is (correctly) rejected as infeasible.

Moving to ASSIGNED to cover adding a regression test for this.

[Bug analyzer/94362] False analyzer report due to i >= 0 and i < 0 on openssl

[dmalcolm at gcc dot gnu.org]

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

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Oops; I was wrong; this isn't yet fixed on trunk.  I can reproduce this with
the attachment.  It also reports warnings from -Wanalyzer-too-complex.

[Bug analyzer/94362] False analyzer report due to i >= 0 and i < 0 on openssl

[dmalcolm at gcc dot gnu.org]

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

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
The root cause is that the analyzer's path feasibility checker erroneously
considers this to be feasible:
  (R + 1 > 0) && (R < 0)
for int R (the return value from sk_EVP_PKEY_ASN1_METHOD_num), whereas it's not
satisfiable for any int R.

I'm working on a fix.

[Bug analyzer/94362] False analyzer report due to i >= 0 and i < 0 on openssl

[cvs-commit at gcc dot gnu.org]

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

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

commit r12-6782-gc4b8f3730a80025192fdb485ad2535c165340e41
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Jan 20 09:51:50 2022 -0500

    analyzer: reject ((i + 1 > 0) && (i < 0)) for integers [PR94362]

    PR analyzer/94362 reports a false positive from
    -Wanalyzer-null-dereference seen when analyzing OpenSSL.

    The root cause is that the analyzer's path feasibility checker
    erroneously considers this to be feasible:
      (R + 1 > 0) && (R < 0)
    for int R (the return value from sk_EVP_PKEY_ASN1_METHOD_num),
    whereas it's not satisfiable for any int R.

    This patch makes the constraint manager try harder to reject
    such combinations of conditions, fixing the false positive;
    perhaps in the longer term we ought to use an SMT solver.

    gcc/analyzer/ChangeLog:
            PR analyzer/94362
            * constraint-manager.cc (bound::ensure_closed): Convert param to
            enum bound_kind.
            (range::constrained_to_single_element): Likewise.
            (range::add_bound): New.
            (constraint_manager::add_constraint): Handle SVAL + OFFSET
            compared to a constant.
            (constraint_manager::get_ec_bounds): Rewrite in terms of
            range::add_bound.
            (constraint_manager::eval_condition): Reject if range::add_bound
            fails.
            (selftest::test_constant_comparisons): Add test coverage for
            various impossible combinations of integer comparisons.
            * constraint-manager.h (enum bound_kind): New.
            (struct bound): Likewise.
            (bound::ensure_closed): Convert to param to enum bound_kind.
            (struct range): Convert to...
            (class range): ...this, making fields private.
            (range::add_bound): New decls.
            * region-model.cc (region_model::add_constraint): Fail if
            constraint_manager::add_constraint fails.

    gcc/testsuite/ChangeLog:
            PR analyzer/94362
            * gcc.dg/analyzer/pr94362-1.c: New test.
            * gcc.dg/analyzer/pr94362-2.c: New test.

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

[Bug analyzer/94362] False analyzer report due to i >= 0 and i < 0 on openssl

[dmalcolm at gcc dot gnu.org]

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

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

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

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed by the above commit (for gcc 12 onwards).

[Bug analyzer/94362] False analyzer report due to i >= 0 and i < 0 on openssl

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 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:e966a508e03fe28bfca65a1e60e579fa90355ea6

commit r12-6875-ge966a508e03fe28bfca65a1e60e579fa90355ea6
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Tue Jan 25 11:35:24 2022 -0500

    analyzer: fix sense in range::add_bound [PR94362]

    Mikael Morin spotted that I got the sense wrong when discarding
    redundant constraints in
    r12-6782-gc4b8f3730a80025192fdb485ad2535c165340e41.

    Fixed as follows, which also moves the rejection of contradictory
    constraints in range::add_bound to earlier, so that this code can
    be self-tested.

    gcc/analyzer/ChangeLog:
            PR analyzer/94362
            * constraint-manager.cc (range::add_bound): Fix tests for
            discarding redundant constraints.  Perform test for rejecting
            unsatisfiable constraints earlier so that they don't update
            the object on failure.
            (selftest::test_range): New.
            (selftest::test_constant_comparisons): Add test coverage for
            existing constraints becoming narrower until they are
            unsatisfiable.
            (selftest::run_constraint_manager_tests): Call test_range.

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