[Bug analyzer/109577] New: -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[Bug analyzer/109577] New: -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[eggert at gnu dot org]

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

            Bug ID: 109577
           Summary: -Wanalyzer-allocation-size mishandles
                    __builtin_mul_overflow
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: eggert at gnu dot org
  Target Milestone: ---

This is (GCC) 13.0.1 20230401 (Red Hat 13.0.1-0) on x86-64.

Compile the following program with 'gcc -O2 -S -fanalyzer t.c'. GCC will
incorrectly complain "warning: allocated buffer size is not a multiple of the
pointee's size [CWE-131]". But the allocated buffer size must be a multiple of
sizeof (double), due to the checked call to __builtin_mul_overflow. As the
code's comment suggests, if the code uses plain * (integer multiply) instead
the bogus warning goes away.

I ran into this problem when compiling Emacs, which is often careful about
checking integer overflow. As a result I think I'll compile Emacs with
-Wno-analyzer-allocation-size to suppress false alarms, which would be a real
shame since this warning is useful for lower-quality code.

  #include <stdlib.h>

  int
  main (int argc, char **argv)
  {
    size_t s;
    double *d;
    if (__builtin_mul_overflow (argc, sizeof *d, &s))
      return 1;
    // No warning if the above is replaced with 's = argc * sizeof *d;'.
    d = malloc (s);
    if (d && s)
      {
        d[0] = argc;
        d[argc - 1] = argc + 1;
        return d[0];
      }
    return 0;
  }

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[eggert at cs dot ucla.edu]

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

Paul Eggert <eggert at cs dot ucla.edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |eggert at cs dot ucla.edu

--- Comment #1 from Paul Eggert <eggert at cs dot ucla.edu> ---
I ran into the same problem with gcc (GCC) 13.1.1 20230426 (Red Hat 13.1.1-1)
but I don't know how to update the version number in Bugzilla.

Also, I came up with the following simpler test case. Compile this with "gcc
-O2 -S -fanalyzer foo.c", and it will complain "allocated buffer size is not a
multiple of the pointee's size" in the function "safer", but it will not
complain about the function "unsafe" (which, unlike "safer", does not check for
integer overflow and so is less safe).

void *malloc (unsigned long);

double *
unsafe (unsigned long n)
{
  return malloc (n * sizeof (double));
}

double *
safer (unsigned long n)
{
  unsigned long nbytes;
  if (__builtin_mul_overflow (n, sizeof (double), &nbytes))
    return 0;
  return malloc (nbytes);
}

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tim Lange <tlange@gcc.gnu.org>:

https://gcc.gnu.org/g:1d57a2232575913ad1085bac0ba5e22b58185179

commit r14-1684-g1d57a2232575913ad1085bac0ba5e22b58185179
Author: Tim Lange <mail@tim-lange.me>
Date:   Fri Jun 9 20:07:33 2023 +0200

    analyzer: Fix allocation size false positive on conjured svalue [PR109577]

    Currently, the analyzer tries to prove that the allocation size is a
    multiple of the pointee's type size.  This patch reverses the behavior
    to try to prove that the expression is not a multiple of the pointee's
    type size.  With this change, each unhandled case should be gracefully
    considered as correct.  This fixes the bug reported in PR 109577 by
    Paul Eggert.

    Regression-tested on Linux x86-64 with -m32 and -m64.

    2023-06-09  Tim Lange  <mail@tim-lange.me>

            PR analyzer/109577

    gcc/analyzer/ChangeLog:

            * constraint-manager.cc (class sval_finder): Visitor to find
            childs in svalue trees.
            (constraint_manager::sval_constrained_p): Add new function to
            check whether a sval might be part of an constraint.
            * constraint-manager.h: Add sval_constrained_p function.
            * region-model.cc (class size_visitor): Reverse behavior to not
            emit a warning on not explicitly considered cases.
            (region_model::check_region_size):
            Adapt to size_visitor changes.

    gcc/testsuite/ChangeLog:

            * gcc.dg/analyzer/allocation-size-2.c: Change expected output
            and add new test case.
            * gcc.dg/analyzer/pr109577.c: New test.

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tim Lange <tlange@gcc.gnu.org>:

https://gcc.gnu.org/g:39adc5eebd61fd276f3f1ef9d7228756a35bd0cb

commit r14-1685-g39adc5eebd61fd276f3f1ef9d7228756a35bd0cb
Author: Tim Lange <mail@tim-lange.me>
Date:   Fri Jun 9 20:08:22 2023 +0200

    testsuite: Add more allocation size tests for conjured svalues [PR110014]

    This patch adds the reproducers reported in PR 110014 as test cases. The
    false positives in those cases are already fixed with PR 109577.

    2023-06-09  Tim Lange  <mail@tim-lange.me>

            PR analyzer/110014

    gcc/testsuite/ChangeLog:

            * gcc.dg/analyzer/realloc-pr110014.c: New tests.

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[dmalcolm at gcc dot gnu.org]

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

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for fixing this Tim.

Keeping open to track backporting this to the gcc 13 branch.

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[nightstrike at gmail dot com]

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

nightstrike <nightstrike at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nightstrike at gmail dot com

--- Comment #5 from nightstrike <nightstrike at gmail dot com> ---
(In reply to David Malcolm from comment #4)
> Thanks for fixing this Tim.
> 
> Keeping open to track backporting this to the gcc 13 branch.

Before this gets backported, the testcase should be fixed.  It uses an
incorrect declaration of malloc, assuming that size_t is long.  The standard
defines malloc as size_t, so the declaration should use __SIZE_TYPE__ instead. 
This works, although you could also just include stdlib.h or use
__builtin_malloc.

[Bug analyzer/109577] [13 Regression] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2024-02-15
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

[Bug analyzer/109577] [13 Regression] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.3

[Bug analyzer/109577] [13 Regression] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by David Malcolm
<dmalcolm@gcc.gnu.org>:

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

commit r13-8742-gccf8d3e3d26c6ba3d5e11fffeed8d64018e9c060
Author: Tim Lange <mail@tim-lange.me>
Date:   Thu May 9 13:09:26 2024 -0400

    analyzer: Fix allocation size false positive on conjured svalue [PR109577]

    Currently, the analyzer tries to prove that the allocation size is a
    multiple of the pointee's type size.  This patch reverses the behavior
    to try to prove that the expression is not a multiple of the pointee's
    type size.  With this change, each unhandled case should be gracefully
    considered as correct.  This fixes the bug reported in PR 109577 by
    Paul Eggert.

    Regression-tested on Linux x86-64 with -m32 and -m64.

    2023-06-09  Tim Lange  <mail@tim-lange.me>

            PR analyzer/109577

    gcc/analyzer/ChangeLog:

            * constraint-manager.cc (class sval_finder): Visitor to find
            childs in svalue trees.
            (constraint_manager::sval_constrained_p): Add new function to
            check whether a sval might be part of an constraint.
            * constraint-manager.h: Add sval_constrained_p function.
            * region-model.cc (class size_visitor): Reverse behavior to not
            emit a warning on not explicitly considered cases.
            (region_model::check_region_size):
            Adapt to size_visitor changes.

    gcc/testsuite/ChangeLog:

            * gcc.dg/analyzer/allocation-size-2.c: Change expected output
            and add new test case.
            * gcc.dg/analyzer/pr109577.c: New test.

    (cherry picked from commit
r14-1684-g1d57a2232575913ad1085bac0ba5e22b58185179)

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

[Bug analyzer/109577] [13 Regression] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by David Malcolm
<dmalcolm@gcc.gnu.org>:

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

commit r13-8743-ge0c52905f666e3d23881f82dbf39466a24f009f4
Author: Tim Lange <mail@tim-lange.me>
Date:   Thu May 9 13:09:26 2024 -0400

    testsuite: Add more allocation size tests for conjured svalues [PR110014]

    This patch adds the reproducers reported in PR 110014 as test cases. The
    false positives in those cases are already fixed with PR 109577.

    2023-06-09  Tim Lange  <mail@tim-lange.me>

            PR analyzer/110014

    gcc/testsuite/ChangeLog:

            * gcc.dg/analyzer/realloc-pr110014.c: New tests.

    (cherry picked from commit
r14-1685-g39adc5eebd61fd276f3f1ef9d7228756a35bd0cb)

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

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED
            Summary|[13 Regression]             |-Wanalyzer-allocation-size
                   |-Wanalyzer-allocation-size  |mishandles
                   |mishandles                  |__builtin_mul_overflow
                   |__builtin_mul_overflow      |

--- Comment #8 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed for GCC 13 (for the upcoming GCC 13.3) by the above patches.

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[nightstrike at gmail dot com]

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

--- Comment #9 from nightstrike <nightstrike at gmail dot com> ---
(In reply to David Malcolm from comment #8)
> Should be fixed for GCC 13 (for the upcoming GCC 13.3) by the above patches.

Did you miss my comment #5 highlighting the need to adjust the declaration of
malloc such that size_t != long?

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[segher at gcc dot gnu.org]

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

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |segher at gcc dot gnu.org
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #10 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Reopened, then.

[Bug analyzer/109577] -Wanalyzer-allocation-size mishandles __builtin_mul_overflow

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.3                        |13.4

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 13.3 is being released, retargeting bugs to GCC 13.4.