Re: [C PATCH]: Add Walloc-type to warn about insufficient size in allocations

[Qing Zhao <qing.zhao@oracle.com>]

> On Aug 1, 2023, at 3:51 AM, Martin Uecker via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> Am Dienstag, dem 01.08.2023 um 02:11 +0530 schrieb Prathamesh Kulkarni:
>> On Fri, 21 Jul 2023 at 16:52, Martin Uecker via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>> 
>>> 
>>> 
>>> This patch adds a warning for allocations with insufficient size
>>> based on the "alloc_size" attribute and the type of the pointer
>>> the result is assigned to. While it is theoretically legal to
>>> assign to the wrong pointer type and cast it to the right type
>>> later, this almost always indicates an error. Since this catches
>>> common mistakes and is simple to diagnose, it is suggested to
>>> add this warning.
>>> 
>>> 
>>> Bootstrapped and regression tested on x86.
>>> 
>>> 
>>> Martin
>>> 
>>> 
>>> 
>>> Add option Walloc-type that warns about allocations that have
>>> insufficient storage for the target type of the pointer the
>>> storage is assigned to.
>>> 
>>> gcc:
>>>        * doc/invoke.texi: Document -Wstrict-flex-arrays option.
>>> 
>>> gcc/c-family:
>>> 
>>>        * c.opt (Walloc-type): New option.
>>> 
>>> gcc/c:
>>>        * c-typeck.cc (convert_for_assignment): Add Walloc-type warning.
>>> 
>>> gcc/testsuite:
>>> 
>>>        * gcc.dg/Walloc-type-1.c: New test.
>>> 
>>> 
>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
>>> index 4abdc8d0e77..8b9d148582b 100644
>>> --- a/gcc/c-family/c.opt
>>> +++ b/gcc/c-family/c.opt
>>> @@ -319,6 +319,10 @@ Walloca
>>> C ObjC C++ ObjC++ Var(warn_alloca) Warning
>>> Warn on any use of alloca.
>>> 
>>> +Walloc-type
>>> +C ObjC Var(warn_alloc_type) Warning
>>> +Warn when allocating insufficient storage for the target type of the
>>> assigned pointer.
>>> +
>>> Walloc-size-larger-than=
>>> C ObjC C++ LTO ObjC++ Var(warn_alloc_size_limit) Joined Host_Wide_Int
>>> ByteSize Warning Init(HOST_WIDE_INT_MAX)
>>> -Walloc-size-larger-than=<bytes>       Warn for calls to allocation
>>> functions that
>>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
>>> index 7cf411155c6..2e392f9c952 100644
>>> --- a/gcc/c/c-typeck.cc
>>> +++ b/gcc/c/c-typeck.cc
>>> @@ -7343,6 +7343,32 @@ convert_for_assignment (location_t location,
>>> location_t expr_loc, tree type,
>>>                    "request for implicit conversion "
>>>                    "from %qT to %qT not permitted in C++", rhstype,
>>> type);
>>> 
>>> +      /* Warn of new allocations are not big enough for the target
>>> type.  */
>>> +      tree fndecl;
>>> +      if (warn_alloc_type
>>> +         && TREE_CODE (rhs) == CALL_EXPR
>>> +         && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
>>> +         && DECL_IS_MALLOC (fndecl))
>>> +       {
>>> +         tree fntype = TREE_TYPE (fndecl);
>>> +         tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
>>> +         tree alloc_size = lookup_attribute ("alloc_size",
>>> fntypeattrs);
>>> +         if (alloc_size)
>>> +           {
>>> +             tree args = TREE_VALUE (alloc_size);
>>> +             int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
>>> +             /* For calloc only use the second argument.  */
>>> +             if (TREE_CHAIN (args))
>>> +               idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN
>>> (args))) - 1;
>>> +             tree arg = CALL_EXPR_ARG (rhs, idx);
>>> +             if (TREE_CODE (arg) == INTEGER_CST
>>> +                 && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
>> Hi Martin,
>> Just wondering if it'd be a good idea perhaps to warn if alloc size is
>> not a multiple of TYPE_SIZE_UNIT instead of just less-than ?
>> So it can catch cases like:
>> int *p = malloc (sizeof (int) + 2); // probably intended malloc
>> (sizeof (int) * 2)
>> 
>> FWIW, this is caught using -fanalyzer:
>> f.c: In function 'f':
>> f.c:3:12: warning: allocated buffer size is not a multiple of the
>> pointee's size [CWE-131] [-Wanalyzer-allocation-size]
>>    3 |   int *p = __builtin_malloc (sizeof(int) + 2);
>>      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 
>> Thanks,
>> Prathamesh
> 
> Yes, this is probably a good idea.  It might need special
> logic for flexible array members then...

Why special logic for FAM on such warning? (Not a multiple of TYPE_SIZE_UNIT for the element).


> 
> 
> Martin
> 
> 
>>> +                warning_at (location, OPT_Walloc_type, "allocation of
>>> "
>>> +                            "insufficient size %qE for type %qT with
>>> "
>>> +                            "size %qE", arg, ttl, TYPE_SIZE_UNIT
>>> (ttl));
>>> +           }
>>> +       }
>>> +
>>>       /* See if the pointers point to incompatible address spaces.  */
>>>       asl = TYPE_ADDR_SPACE (ttl);
>>>       asr = TYPE_ADDR_SPACE (ttr);
>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>>> index 88e3c625030..6869bed64c3 100644
>>> --- a/gcc/doc/invoke.texi
>>> +++ b/gcc/doc/invoke.texi
>>> @@ -8076,6 +8076,15 @@ always leads to a call to another @code{cold}
>>> function such as wrappers of
>>> C++ @code{throw} or fatal error reporting functions leading to
>>> @code{abort}.
>>> @end table
>>> 
>>> +@opindex Wno-alloc-type
>>> +@opindex Walloc-type
>>> +@item -Walloc-type
>>> +Warn about calls to allocation functions decorated with attribute
>>> +@code{alloc_size} that specify insufficient size for the target type
>>> of
>>> +the pointer the result is assigned to, including those to the built-in
>>> +forms of the functions @code{aligned_alloc}, @code{alloca},
>>> @code{calloc},
>>> +@code{malloc}, and @code{realloc}.
>>> +
>>> @opindex Wno-alloc-zero
>>> @opindex Walloc-zero
>>> @item -Walloc-zero
>>> diff --git a/gcc/testsuite/gcc.dg/Walloc-type-1.c
>>> b/gcc/testsuite/gcc.dg/Walloc-type-1.c
>>> new file mode 100644
>>> index 00000000000..bc62e5e9aa3
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/Walloc-type-1.c
>>> @@ -0,0 +1,37 @@
>>> +/* Tests the warnings for insufficient allocation size.
>>> +   { dg-do compile }
>>> + * { dg-options "-Walloc-type" }
>>> + * */
>>> +#include <stdlib.h>
>>> +#include <alloca.h>
>>> +
>>> +struct b { int x[10]; };
>>> +
>>> +void fo0(void)
>>> +{
>>> +        struct b *p = malloc(sizeof *p);
>>> +}
>>> +
>>> +void fo1(void)
>>> +{
>>> +        struct b *p = malloc(sizeof p);                /* { dg-
>>> warning "allocation of insufficient size" } */
>>> +}
>>> +
>>> +void fo2(void)
>>> +{
>>> +        struct b *p = alloca(sizeof p);                /* { dg-
>>> warning "allocation of insufficient size" } */
>>> +}
>>> +
>>> +void fo3(void)
>>> +{
>>> +        struct b *p = calloc(1, sizeof p);     /* { dg-warning
>>> "allocation of insufficient size" } */
>>> +}
>>> +
>>> +void g(struct b* p);
>>> +
>>> +void fo4(void)
>>> +{
>>> +        g(malloc(4));          /* { dg-warning "allocation of
>>> insufficient size" } */
>>> +}
>>> +
>>> +
>>> 
>>> 
>>> 
> 
> -- 
> Univ.-Prof. Dr. rer. nat. Martin Uecker
> Graz University of Technology
> Institute of Biomedical Imaging