Re: Floating-point allocation sizes? (was Re: [PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181])
On Tue, Jul 5 2022 at 05:37:46 PM -0400, David Malcolm
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.
While fixing that case, I thought what sane person would think of using
floating-point arithmetic here. The main reason why I chose to give up
here instead of complain was because the checker can't know the result
and it is strange enough such that it might be deliberately.
In that sense, we could also talk about allocating 0 bytes. What
happens there seems to be undefined behavior and
implementation-specific. I've again decided to say that allocating 0
bytes is strange enough that it must be deliberately. The same standard
you've linked also has a article about that case [0]. If all readers
can't thing of any use case, I can certainly rework that patch to warn
on such allocations.
- Tim
[0]
https://wiki.sei.cmu.edu/confluence/display/c/MEM04-C.+Beware+of+zero-length+allocations
I also wondered what happens on NAN, with e.g.
#include
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).
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
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
Re: Floating-point allocation sizes? (was Re: [PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181])
On Wed, 6 Jul 2022 at 03:08, David Malcolm via Gcc-patches
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
>
> 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
> >
> > 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
Floating-point allocation sizes? (was Re: [PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181])
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
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).
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
>
> 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, ))
> + return true;
> + alloc_size = i;
> + }
> + }
> +
[PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181]
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.
Regression-tested on x86_64 linux.
2022-07-05 Tim Lange
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, ))
+ 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 (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);
+
