Re: Floating-point allocation sizes? (was Re: [PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181])

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

On Wed, 6 Jul 2022 at 03:08, David Malcolm via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Tue, 2022-07-05 at 21:49 +0200, Tim Lange wrote:
> > This patch fixes the ICE reported in PR106181 by Arseny Solokha. With
> > this patch, the allocation size checker tries to handle floating-point
> > operands of allocation size arguments. Constant sizes get implicitly
> > converted and symbolic sizes are handled as long as the floating-point
> > operand could also be represented as a positive integer. In all other
> > cases and on unhandled constants, the checker falls back to not
> > emitting a warning.
> > Also, I unified the logic on zero byte allocations.
>
> Hi Tim
>
> Thanks for the patch.
>
> We definitely don't want to crash, but my "gut reaction" to the
> testsuite examples was that we ought to be warning on them - using
> floating point when calculating an allocation size seems like asking
> for trouble.
>
> In particular test_16's:
>   int32_t *ptr = malloc (n * 3.1);
> feels to me like it deserves a warning.  I suppose it could be valid if
> n is a multiple of 40 (so that the buffer is a multiple of 31 * 4 and
> thus a multiple of 4), for small enough n that we don't lose precision,
> but that code seems very questionable - the comment says "just assume
> that the programmer knows what they are doing", but I think anyone
> using -fanalyzer is opting-in to have more fussy checking and would
> probably want to be warned about such code.
>
> I also wondered what happens on NAN, with e.g.
>
> #include <math.h>
>
> void test_nan (void)
> {
>   int *p = malloc (NAN * sizeof (int));
> }
>
> but we issue -Woverflow on that.
>
> I'm thinking that perhaps we should have a new warning for floating
> point buffer size calculations, though I'm not yet sure exactly how it
> should work and how fussy it should be (e.g. complain about floating
> point calculations vs complain about *any* floating point used as a
> buffer size, etc).
Hi David,
For the former, I was wondering if it'd be a good idea to complain if
alloc_size is floating type,
and floor(alloc_size) != alloc_size ?
So we warn for:
int *p = malloc(4.5);
but not for
int *p = malloc(4.0);

For the latter, I guess -Wconversion should be sufficient ?

Thanks,
Prathamesh
>
> Does anyone know of real world code that uses floating point in buffer-
> size calculations?  (updating Subject accordingly)  Is there code out
> there that does this?  It seems broken to me, but maybe there's a valid
> use-case that I can't think of.
>
> The closest such rule I can think of is CERT-C's
> "FLP02-C. Avoid using floating-point numbers when precise computation
> is needed":
> https://wiki.sei.cmu.edu/confluence/display/c/FLP02-C.+Avoid+using+floating-point+numbers+when+precise+computation+is+needed
>
>
> Dave
>
>
> >
> > Regression-tested on x86_64 linux.
> >
> > 2022-07-05  Tim Lange  <mail@tim-lange.me>
> >
> > gcc/analyzer/ChangeLog:
> >
> >         PR analyzer/106181
> >         * region-model.cc (capacity_compatible_with_type):
> >         Can handle non-integer constants now.
> >         (region_model::check_region_size): Adapted to the new signature
> > of
> >         capacity_compatible_with_type.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         PR analyzer/106181
> >         * gcc.dg/analyzer/allocation-size-1.c: New tests.
> >         * gcc.dg/analyzer/allocation-size-2.c: New tests.
> >         * gcc.dg/analyzer/pr106181.c: New test.
> >
> > ---
> >  gcc/analyzer/region-model.cc                  | 44 ++++++++++++++++---
> >  .../gcc.dg/analyzer/allocation-size-1.c       | 29 +++++++++++-
> >  .../gcc.dg/analyzer/allocation-size-2.c       | 22 ++++++++++
> >  gcc/testsuite/gcc.dg/analyzer/pr106181.c      |  7 +++
> >  4 files changed, 95 insertions(+), 7 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c
> >
> > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
> > model.cc
> > index 5d939327e01..e097ecb3c07 100644
> > --- a/gcc/analyzer/region-model.cc
> > +++ b/gcc/analyzer/region-model.cc
> > @@ -2904,13 +2904,45 @@ private:
> >
> >  static bool
> >  capacity_compatible_with_type (tree cst, tree pointee_size_tree,
> > -                              bool is_struct)
> > +                              bool is_struct, bool floor_real)
> >  {
> > -  gcc_assert (TREE_CODE (cst) == INTEGER_CST);
> >    gcc_assert (TREE_CODE (pointee_size_tree) == INTEGER_CST);
> > -
> >    unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW
> > (pointee_size_tree);
> > -  unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst);
> > +
> > +  unsigned HOST_WIDE_INT alloc_size;
> > +  switch (TREE_CODE (cst))
> > +    {
> > +    default:
> > +      /* Assume all unhandled operands are compatible.  */
> > +      return true;
> > +    case INTEGER_CST:
> > +      alloc_size = TREE_INT_CST_LOW (cst);
> > +      break;
> > +    case REAL_CST:
> > +      {
> > +       const REAL_VALUE_TYPE *rv = TREE_REAL_CST_PTR (cst);
> > +       if (floor_real)
> > +         {
> > +           /* If the size is constant real at compile-time,
> > +              we can model the conversion.  */
> > +           alloc_size = real_to_integer (rv);
> > +         }
> > +       else
> > +         {
> > +           /* On expressions where the value of one operator isn't
> > +              representable as an integer or is negative, we give up
> > and
> > +              just assume that the programmer knows what they are
> > doing.  */
> > +           HOST_WIDE_INT i;
> > +           if (real_isneg (rv) || !real_isinteger (rv, &i))
> > +             return true;
> > +           alloc_size = i;
> > +         }
> > +      }
> > +      break;
> > +    }
> > +
> > +  if (alloc_size == 0)
> > +    return true;
> >
> >    if (is_struct)
> >      return alloc_size >= pointee_size;
> > @@ -2920,7 +2952,7 @@ capacity_compatible_with_type (tree cst, tree
> > pointee_size_tree,
> >  static bool
> >  capacity_compatible_with_type (tree cst, tree pointee_size_tree)
> >  {
> > -  return capacity_compatible_with_type (cst, pointee_size_tree,
> > false);
> > +  return capacity_compatible_with_type (cst, pointee_size_tree, false,
> > false);
> >  }
> >
> >  /* Checks whether SVAL could be a multiple of SIZE_CST.
> > @@ -3145,7 +3177,7 @@ region_model::check_region_size (const region
> > *lhs_reg, const svalue *rhs_sval,
> >                 = as_a <const constant_svalue *> (capacity);
> >         tree cst_cap = cst_cap_sval->get_constant ();
> >         if (!capacity_compatible_with_type (cst_cap, pointee_size_tree,
> > -                                           is_struct))
> > +                                           is_struct, true))
> >           ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg,
> >                                                    cst_cap));
> >        }
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
> > b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
> > index 4a78a81d054..1a1c8e75c98 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
> > +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
> > @@ -114,4 +114,31 @@ void test_10 (int32_t n)
> >  {
> >    char *ptr = malloc (7 * n);
> >    free (ptr);
> > -}
> > \ No newline at end of file
> > +}
> > +
> > +void test_11 ()
> > +{
> > +  int32_t *ptr = malloc (3.0); /* { dg-line malloc11 } */
> > +  free (ptr);
> > +  /* { dg-warning "allocated buffer size is not a multiple of the
> > pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc11 }
> > */
> > +  /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})?
> > here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note"
> > { target *-*-* } malloc11 } */
> > +}
> > +
> > +void test_12 ()
> > +{
> > +  int32_t *ptr = malloc (4.0);
> > +  free (ptr);
> > +}
> > +
> > +void test_13 ()
> > +{
> > +  int32_t *ptr = malloc (4.7);
> > +  free (ptr);
> > +}
> > +
> > +void test_14 ()
> > +{
> > +  /* Test round towards zero.  */
> > +  int32_t *ptr = malloc (-0.9);
> > +  free (ptr);
> > +}
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
> > b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
> > index d541d5ef8db..babf9ae668d 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
> > +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
> > @@ -154,3 +154,25 @@ void test_13 (void)
> >    else
> >      free (ptr);
> >  }
> > +
> > +void test_14 (int32_t n)
> > +{
> > +  int32_t *ptr = malloc (n * 3.0); /* { dg-line malloc14 } */
> > +  free (ptr);
> > +  /* { dg-warning "allocated buffer size is not a multiple of the
> > pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc14 }
> > */
> > +  /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})?
> > here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note"
> > { target *-*-* } malloc14 } */
> > +}
> > +
> > +void test_15 (int32_t n)
> > +{
> > +  int32_t *ptr = malloc (n * 4.0);
> > +  free (ptr);
> > +}
> > +
> > +void test_16 (int32_t n)
> > +{
> > +  /* Should not emit a warning because we can not reason whether the
> > result
> > +     of the floating-point arithmetic actually is a valid size or
> > not.  */
> > +  int32_t *ptr = malloc (n * 3.1);
> > +  free (ptr);
> > +}
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/pr106181.c
> > b/gcc/testsuite/gcc.dg/analyzer/pr106181.c
> > new file mode 100644
> > index 00000000000..6df4e4538c0
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/analyzer/pr106181.c
> > @@ -0,0 +1,7 @@
> > +#include <stdint.h>
> > +
> > +int32_t *
> > +foo (int32_t x)
> > +{
> > +  return __builtin_calloc (x * 1.1, 1);
> > +}
>
>