[PATCH] analyzer: Bail out on function pointer for -Wanalyzer-allocation-size

[PATCH] analyzer: Bail out on function pointer for -Wanalyzer-allocation-size

[Stefan Schulze Frielinghaus]

On s390 pr94688.c is failing due to excess error

pr94688.c:6:5: warning: allocated buffer size is not a multiple of the pointee's size [CWE-131] [-Wanalyzer-allocation-size]

This is because on s390 functions are by default aligned to an 8-byte
boundary and during function type construction size is set to function
boundary.  Thus, for the assignment

a.0_1 = (void (*<T237>) ()) &a;

we have that the right-hand side is pointing to a 4-byte memory region
whereas the size of the function pointer is 8 byte and a warning is
emitted.

I could follow and skip this test as done in PR112705, or we could bail
out early in the analyzer for function pointers.  My intuition so far
is that -Wanalyzer-allocation-size shouldn't care about function
pointer.  Therefore, I went for bailing out early.  If you believe this
is wrong I can still go by skipping this test on s390.  Any thoughts?
---
 gcc/analyzer/region-model.cc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index f079d1fb37e..1b43443d168 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3514,6 +3514,10 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval,
       || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE)
     return;
 
+  /* Bail out early on function pointers.  */
+  if (TREE_CODE (pointee_type) == FUNCTION_TYPE)
+    return;
+
   /* Bail out early on pointers to structs where we can
      not deduce whether the buffer size is compatible.  */
   bool is_struct = RECORD_OR_UNION_TYPE_P (pointee_type);
-- 
2.43.0

Re: [PATCH] analyzer: Bail out on function pointer for -Wanalyzer-allocation-size

[David Malcolm]

On Tue, 2024-03-19 at 16:10 +0100, Stefan Schulze Frielinghaus wrote:
> On s390 pr94688.c is failing due to excess error
> 
> pr94688.c:6:5: warning: allocated buffer size is not a multiple of
> the pointee's size [CWE-131] [-Wanalyzer-allocation-size]
> 
> This is because on s390 functions are by default aligned to an 8-byte
> boundary and during function type construction size is set to
> function
> boundary.  Thus, for the assignment
> 
> a.0_1 = (void (*<T237>) ()) &a;
> 
> we have that the right-hand side is pointing to a 4-byte memory
> region
> whereas the size of the function pointer is 8 byte and a warning is
> emitted.

FWIW the test case in question is a regression test for an ICE seen in
the GCC 10 implementation of the analyzer, which was fixed by the big
rewrite in r11-2694-g808f4dfeb3a95f.

So the code in the test doesn't make a great deal of sense.

> 
> I could follow and skip this test as done in PR112705, or we could
> bail
> out early in the analyzer for function pointers.  My intuition so far
> is that -Wanalyzer-allocation-size shouldn't care about function
> pointer.  Therefore, I went for bailing out early.  If you believe
> this
> is wrong I can still go by skipping this test on s390.  Any thoughts?

I tried imagining a situation where we're analyzing a function
generated at run-time, but it strikes me that the buffer allocated for
such a function can be of arbitrary size.  So -Wanalyzer-allocation-
size is meaningless for functions.

There's probably a case for checking for mismatches between pointers to
code vs pointers to data (e.g. alignments, Harvard architecture
machines, etc), but -Wanalyzer-allocation-size doesn't do that.

So I think your patch is correct.

OK to push it if it passes bootstrap&regression testing.

Thanks
Dave

> ---
>  gcc/analyzer/region-model.cc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
> model.cc
> index f079d1fb37e..1b43443d168 100644
> --- a/gcc/analyzer/region-model.cc
> +++ b/gcc/analyzer/region-model.cc
> @@ -3514,6 +3514,10 @@ region_model::check_region_size (const region
> *lhs_reg, const svalue *rhs_sval,
>        || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE)
>      return;
>  
> +  /* Bail out early on function pointers.  */
> +  if (TREE_CODE (pointee_type) == FUNCTION_TYPE)
> +    return;
> +
>    /* Bail out early on pointers to structs where we can
>       not deduce whether the buffer size is compatible.  */
>    bool is_struct = RECORD_OR_UNION_TYPE_P (pointee_type);

Re: [PATCH] analyzer: Bail out on function pointer for -Wanalyzer-allocation-size

[Stefan Schulze Frielinghaus]

On Tue, Mar 19, 2024 at 12:38:34PM -0400, David Malcolm wrote:
> On Tue, 2024-03-19 at 16:10 +0100, Stefan Schulze Frielinghaus wrote:
> > On s390 pr94688.c is failing due to excess error
> > 
> > pr94688.c:6:5: warning: allocated buffer size is not a multiple of
> > the pointee's size [CWE-131] [-Wanalyzer-allocation-size]
> > 
> > This is because on s390 functions are by default aligned to an 8-byte
> > boundary and during function type construction size is set to
> > function
> > boundary.  Thus, for the assignment
> > 
> > a.0_1 = (void (*<T237>) ()) &a;
> > 
> > we have that the right-hand side is pointing to a 4-byte memory
> > region
> > whereas the size of the function pointer is 8 byte and a warning is
> > emitted.
> 
> FWIW the test case in question is a regression test for an ICE seen in
> the GCC 10 implementation of the analyzer, which was fixed by the big
> rewrite in r11-2694-g808f4dfeb3a95f.
> 
> So the code in the test doesn't make a great deal of sense.
> 
> > 
> > I could follow and skip this test as done in PR112705, or we could
> > bail
> > out early in the analyzer for function pointers.  My intuition so far
> > is that -Wanalyzer-allocation-size shouldn't care about function
> > pointer.  Therefore, I went for bailing out early.  If you believe
> > this
> > is wrong I can still go by skipping this test on s390.  Any thoughts?
> 
> I tried imagining a situation where we're analyzing a function
> generated at run-time, but it strikes me that the buffer allocated for
> such a function can be of arbitrary size.  So -Wanalyzer-allocation-
> size is meaningless for functions.
> 
> There's probably a case for checking for mismatches between pointers to
> code vs pointers to data (e.g. alignments, Harvard architecture
> machines, etc), but -Wanalyzer-allocation-size doesn't do that.
> 
> So I think your patch is correct.
> 
> OK to push it if it passes bootstrap&regression testing.

Bootstrapped and regtested on x64 and s390x.

Thanks,
Stefan

> 
> Thanks
> Dave
> 
> > ---
> >  gcc/analyzer/region-model.cc | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
> > model.cc
> > index f079d1fb37e..1b43443d168 100644
> > --- a/gcc/analyzer/region-model.cc
> > +++ b/gcc/analyzer/region-model.cc
> > @@ -3514,6 +3514,10 @@ region_model::check_region_size (const region
> > *lhs_reg, const svalue *rhs_sval,
> >        || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE)
> >      return;
> >  
> > +  /* Bail out early on function pointers.  */
> > +  if (TREE_CODE (pointee_type) == FUNCTION_TYPE)
> > +    return;
> > +
> >    /* Bail out early on pointers to structs where we can
> >       not deduce whether the buffer size is compatible.  */
> >    bool is_struct = RECORD_OR_UNION_TYPE_P (pointee_type);
>