[C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Compared to the previous version I changed the name of the warning to "Walloc-size" which matches "Wanalyzer-allocation-size" but is still in line with the other -Walloc-something warnings we have. I also added it to Wextra. I found PR71219 that requests the warning and points out that it is recommended by the C secure coding guidelines and added the PR to the commit log (although the version with cast is not diagnosed so far.) I did not have time to implement the extensions suggested on the list, i.e. warn when the size is not a multiple of the size of the type and warn for if the size is not suitable for a flexible array member. (this is also a bit more complicated than it seems) Bootstrapped and regression tested on x86_64. Martin Add option Walloc-size that warns about allocations that have insufficient storage for the target type of the pointer the storage is assigned to. PR c/71219 gcc: * doc/invoke.texi: Document -Walloc-size option. gcc/c-family: * c.opt (Walloc-size): New option. gcc/c: * c-typeck.cc (convert_for_assignment): Add warning. gcc/testsuite: * gcc.dg/Walloc-size-1.c: New test. --- gcc/c-family/c.opt | 4 gcc/c/c-typeck.cc| 27 + gcc/doc/invoke.texi | 10 gcc/testsuite/gcc.dg/Walloc-size-1.c | 36 4 files changed, 77 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/Walloc-size-1.c diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 7348ad42ee0..9ba08a1fb6d 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-size +C ObjC Var(warn_alloc_size) 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= Warn for calls to allocation functions that diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index e2bfd2caf85..c759c6245ed 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -7384,6 +7384,33 @@ 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 that are not big enough for the target +type. */ + tree fndecl; + if (warn_alloc_size + && 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))) +warning_at (location, OPT_Walloc_size, "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 33befee7d6b..a4fbcf5e1b5 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -8086,6 +8086,16 @@ 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-size +@opindex Walloc-size +@item -Walloc-size +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-size-1.c b/gcc/testsuite/gcc.dg/Walloc-size-1.c new file mode 100644 index 000..61806f58192 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Walloc-size-1.c @@ -0,0 +1,36 @@ +/* Tests the warnings for insufficient allocation size. + { dg-do compile } + { dg-options "-Walloc-size" } + * */ +#include +#include + +struct
[PING] [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Am Montag, dem 18.09.2023 um 23:26 +0200 schrieb Martin Uecker: > > Compared to the previous version I changed the name of the > warning to "Walloc-size" which matches "Wanalyzer-allocation-size" > but is still in line with the other -Walloc-something warnings > we have. I also added it to Wextra. > > I found PR71219 that requests the warning and points out that > it is recommended by the C secure coding guidelines and added > the PR to the commit log (although the version with cast is not > diagnosed so far.) > > I did not have time to implement the extensions suggested > on the list, i.e. warn when the size is not a multiple > of the size of the type and warn for if the size is not > suitable for a flexible array member. (this is also a bit > more complicated than it seems) > > Bootstrapped and regression tested on x86_64. > > > Martin > > > Add option Walloc-size that warns about allocations that have > insufficient storage for the target type of the pointer the > storage is assigned to. > > PR c/71219 > gcc: > * doc/invoke.texi: Document -Walloc-size option. > > gcc/c-family: > > * c.opt (Walloc-size): New option. > > gcc/c: > * c-typeck.cc (convert_for_assignment): Add warning. > > gcc/testsuite: > > * gcc.dg/Walloc-size-1.c: New test. > --- > gcc/c-family/c.opt | 4 > gcc/c/c-typeck.cc| 27 + > gcc/doc/invoke.texi | 10 > gcc/testsuite/gcc.dg/Walloc-size-1.c | 36 > 4 files changed, 77 insertions(+) > create mode 100644 gcc/testsuite/gcc.dg/Walloc-size-1.c > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index 7348ad42ee0..9ba08a1fb6d 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-size > +C ObjC Var(warn_alloc_size) 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= Warn for calls to allocation functions > that > diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > index e2bfd2caf85..c759c6245ed 100644 > --- a/gcc/c/c-typeck.cc > +++ b/gcc/c/c-typeck.cc > @@ -7384,6 +7384,33 @@ 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 that are not big enough for the target > + type. */ > + tree fndecl; > + if (warn_alloc_size > + && 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))) > + warning_at (location, OPT_Walloc_size, "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 33befee7d6b..a4fbcf5e1b5 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -8086,6 +8086,16 @@ 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-size > +@opindex Walloc-size > +@item -Walloc-size > +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-size-1.c > b/gcc/testsuite/gcc.dg/Walloc-size-1.c > new file mode 10
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Mon, Sep 18, 2023 at 11:26:49PM +0200, Martin Uecker via Gcc-patches wrote: > Add option Walloc-size that warns about allocations that have > insufficient storage for the target type of the pointer the > storage is assigned to. > > PR c/71219 > gcc: > * doc/invoke.texi: Document -Walloc-size option. > > gcc/c-family: > > * c.opt (Walloc-size): New option. > > gcc/c: > * c-typeck.cc (convert_for_assignment): Add warning. > > gcc/testsuite: > > * gcc.dg/Walloc-size-1.c: New test. > --- a/gcc/c/c-typeck.cc > +++ b/gcc/c/c-typeck.cc > @@ -7384,6 +7384,33 @@ 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 that are not big enough for the target > + type. */ > + tree fndecl; > + if (warn_alloc_size > + && 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; I wonder if this part isn't too pedantic or more of a code style. Some packages fail to build with this with -Werror because they do struct S *p = calloc (sizeof (struct S), 1); or similar. It is true that calloc arguments are documented to be nmemb, size, but given sufficient alignment (which is not really different between either order of arguments) isn't it completely valid to allocate char array with sizeof (struct S) elements and then store a struct S object into it? Given struct S { int a, b; int flex[]; }; struct S *a = calloc (sizeof (struct S), 1); struct S *b = calloc (1, sizeof (struct S)); struct S *c = calloc (sizeof (struct S), n); struct S *d = calloc (n, sizeof (struct S)); struct S *e = calloc (n * sizeof (struct S), 1); struct S *f = calloc (1, n * sizeof (struct S)); struct S *g = calloc (offsetof (struct S, flex[0]) + n * sizeof (int), 1); struct S *h = calloc (1, offsetof (struct S, flex[0]) + n * sizeof (int)); what from these feels like a potentially dangerous use compared to just style that some project might want to enforce? I'd say b, d, h deserve no warning at all, the rest are just style warnings (e and f might have warnings desirable as a potential security problem because it doesn't deal with overflows). So, for -Walloc-size as written, wouldn't it be better to check for calloc both arguments, if both are constant, either just multiply them and check the product or check if both are smaller than TYPE_SIZE_UNIT, if only one of them is constant, check that one with possible exception of 1 (in which case try say if the other argument is multiple_of_p or something similar)? > + tree arg = CALL_EXPR_ARG (rhs, idx); > + if (TREE_CODE (arg) == INTEGER_CST > + && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl))) > + warning_at (location, OPT_Walloc_size, "allocation of " > + "insufficient size %qE for type %qT with " > + "size %qE", arg, ttl, TYPE_SIZE_UNIT (ttl)); > + } > + } > + Jakub
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote: > I wonder if this part isn't too pedantic or more of a code style. > Some packages fail to build with this with -Werror because they do > struct S *p = calloc (sizeof (struct S), 1); > or similar. It is true that calloc arguments are documented to be > nmemb, size, but given sufficient alignment (which is not really different > between either order of arguments) isn't it completely valid to allocate > char array with sizeof (struct S) elements and then store a struct S object > into it? In PR112364 Martin Uecker has pointed out the alignment may be different with the different order of arguments, per C23 (N2293). With earlier versions of the standard some people believe the alignment should not be different, while the other people disagree (as the text is not very clear). -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Wed, Dec 06, 2023 at 08:31:12PM +0800, Xi Ruoyao wrote: > On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote: > > I wonder if this part isn't too pedantic or more of a code style. > > Some packages fail to build with this with -Werror because they do > > struct S *p = calloc (sizeof (struct S), 1); > > or similar. It is true that calloc arguments are documented to be > > nmemb, size, but given sufficient alignment (which is not really different > > between either order of arguments) isn't it completely valid to allocate > > char array with sizeof (struct S) elements and then store a struct S object > > into it? > > In PR112364 Martin Uecker has pointed out the alignment may be different > with the different order of arguments, per C23 (N2293). With earlier > versions of the standard some people believe the alignment should not be > different, while the other people disagree (as the text is not very > clear). I can understand implementations which use smaller alignment based on allocation size, but are there any which consider for that just the second calloc argument rather than the product of both arguments? I think they'd quickly break a lot of real-world code. Further I think "size less than or equal to the size requested" is quite ambiguous in the calloc case, isn't the size requested in the calloc case actually nmemb * size rather than just size? Jakub
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Am Mittwoch, dem 06.12.2023 um 13:57 +0100 schrieb Jakub Jelinek: > On Wed, Dec 06, 2023 at 08:31:12PM +0800, Xi Ruoyao wrote: > > On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote: > > > I wonder if this part isn't too pedantic or more of a code style. > > > Some packages fail to build with this with -Werror because they do > > > struct S *p = calloc (sizeof (struct S), 1); > > > or similar. It is true that calloc arguments are documented to be > > > nmemb, size, but given sufficient alignment (which is not really different > > > between either order of arguments) isn't it completely valid to allocate > > > char array with sizeof (struct S) elements and then store a struct S > > > object > > > into it? > > > > In PR112364 Martin Uecker has pointed out the alignment may be different > > with the different order of arguments, per C23 (N2293). With earlier > > versions of the standard some people believe the alignment should not be > > different, while the other people disagree (as the text is not very > > clear). > > I can understand implementations which use smaller alignment based on > allocation size, but are there any which consider for that just the second > calloc argument rather than the product of both arguments? Not that I know of. > I think they'd quickly break a lot of real-world code. There are quite a few projects which use calloc with swapped arguments. > Further I think > "size less than or equal to the size requested" > is quite ambiguous in the calloc case, isn't the size requested in the > calloc case actually nmemb * size rather than just size? This is unclear but it can be understood this way. This was also Joseph's point. I am happy to submit a patch that changes the code so that the swapped arguments to calloc do not cause a warning anymore. On the other hand, the only feedback I got so far was from people who were then happy to get this warning. Martin
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Am Mittwoch, dem 06.12.2023 um 14:34 +0100 schrieb Martin Uecker: > Am Mittwoch, dem 06.12.2023 um 13:57 +0100 schrieb Jakub Jelinek: > > On Wed, Dec 06, 2023 at 08:31:12PM +0800, Xi Ruoyao wrote: > > > On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote: > > > > I wonder if this part isn't too pedantic or more of a code style. > > > > Some packages fail to build with this with -Werror because they do > > > > struct S *p = calloc (sizeof (struct S), 1); > > > > or similar. It is true that calloc arguments are documented to be > > > > nmemb, size, but given sufficient alignment (which is not really > > > > different > > > > between either order of arguments) isn't it completely valid to allocate > > > > char array with sizeof (struct S) elements and then store a struct S > > > > object > > > > into it? > > > > > > In PR112364 Martin Uecker has pointed out the alignment may be different > > > with the different order of arguments, per C23 (N2293). With earlier > > > versions of the standard some people believe the alignment should not be > > > different, while the other people disagree (as the text is not very > > > clear). > > > > I can understand implementations which use smaller alignment based on > > allocation size, but are there any which consider for that just the second > > calloc argument rather than the product of both arguments? > > Not that I know of. > > > I think they'd quickly break a lot of real-world code. > > There are quite a few projects which use calloc with swapped > arguments. > > > Further I think > > "size less than or equal to the size requested" > > is quite ambiguous in the calloc case, isn't the size requested in the > > calloc case actually nmemb * size rather than just size? > > This is unclear but it can be understood this way. > This was also Joseph's point. > > I am happy to submit a patch that changes the code so > that the swapped arguments to calloc do not cause a warning > anymore. > > On the other hand, the only feedback I got so far was > from people who were then happy to get this warning. Note that it is now -Wextra.
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Wed, Dec 06, 2023 at 02:34:10PM +0100, Martin Uecker wrote: > > Further I think > > "size less than or equal to the size requested" > > is quite ambiguous in the calloc case, isn't the size requested in the > > calloc case actually nmemb * size rather than just size? > > This is unclear but it can be understood this way. > This was also Joseph's point. > > I am happy to submit a patch that changes the code so > that the swapped arguments to calloc do not cause a warning > anymore. That would be my preference because then the allocation size is correct and it is purely a style warning. It doesn't follow how the warning is described: "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" when the size is certainly sufficient. But wonder what others think about it. BTW, shouldn't the warning be for C++ as well? Sure, I know, people use operator new more often, but still, the allocators are used in there as well. We have the -Wmemset-transposed-args warning, couldn't we have a similar one for calloc, and perhaps do it solely in the case where one uses sizeof of the type used in the cast pointer? So warn for (struct S *) calloc (sizeof (struct S), 1) or (struct S *) calloc (sizeof (struct S), n) but not for (struct S *) calloc (4, 15) or (struct S *) calloc (sizeof (struct T), 1) or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. Jakub
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On 2023-12-06 09:21, Jakub Jelinek wrote: On Wed, Dec 06, 2023 at 02:34:10PM +0100, Martin Uecker wrote: Further I think "size less than or equal to the size requested" is quite ambiguous in the calloc case, isn't the size requested in the calloc case actually nmemb * size rather than just size? This is unclear but it can be understood this way. This was also Joseph's point. I am happy to submit a patch that changes the code so that the swapped arguments to calloc do not cause a warning anymore. That would be my preference because then the allocation size is correct and it is purely a style warning. It doesn't follow how the warning is described: "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" when the size is certainly sufficient. But wonder what others think about it. +1, from a libc perspective, the transposed arguments don't make a difference, a typical allocator will produce sufficiently sized allocation for the calloc call. BTW, shouldn't the warning be for C++ as well? Sure, I know, people use operator new more often, but still, the allocators are used in there as well. We have the -Wmemset-transposed-args warning, couldn't we have a similar one for calloc, and perhaps do it solely in the case where one uses sizeof of the type used in the cast pointer? So warn for (struct S *) calloc (sizeof (struct S), 1) or (struct S *) calloc (sizeof (struct S), n) but not for (struct S *) calloc (4, 15) or (struct S *) calloc (sizeof (struct T), 1) or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. +1, this could be an analyzer warning, since in practice it is just a code cleanliness issue. Thanks, Sid
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Wed, Dec 06, 2023 at 09:30:32AM -0500, Siddhesh Poyarekar wrote: > > We have the -Wmemset-transposed-args warning, couldn't we > > have a similar one for calloc, and perhaps do it solely in > > the case where one uses sizeof of the type used in the cast > > pointer? > > So warn for > > (struct S *) calloc (sizeof (struct S), 1) > > or > > (struct S *) calloc (sizeof (struct S), n) > > but not for > > (struct S *) calloc (4, 15) > > or > > (struct S *) calloc (sizeof (struct T), 1) > > or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. > > +1, this could be an analyzer warning, since in practice it is just a code > cleanliness issue. We don't do such things in the analyzer, nor it is possible, by the time analyzer sees the IL all the sizeofs etc. are folded. Analyzer is for expensive to compute warnings, code style warnings are normally implemented in the FEs. Jakub
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On 2023-12-06 09:41, Jakub Jelinek wrote: On Wed, Dec 06, 2023 at 09:30:32AM -0500, Siddhesh Poyarekar wrote: We have the -Wmemset-transposed-args warning, couldn't we have a similar one for calloc, and perhaps do it solely in the case where one uses sizeof of the type used in the cast pointer? So warn for (struct S *) calloc (sizeof (struct S), 1) or (struct S *) calloc (sizeof (struct S), n) but not for (struct S *) calloc (4, 15) or (struct S *) calloc (sizeof (struct T), 1) or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. +1, this could be an analyzer warning, since in practice it is just a code cleanliness issue. We don't do such things in the analyzer, nor it is possible, by the time analyzer sees the IL all the sizeofs etc. are folded. Analyzer is for expensive to compute warnings, code style warnings are normally implemented in the FEs. Thanks, understood. A separate FE warning is fine as well. Thanks, Sid
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Am Mittwoch, dem 06.12.2023 um 15:21 +0100 schrieb Jakub Jelinek: > On Wed, Dec 06, 2023 at 02:34:10PM +0100, Martin Uecker wrote: > > > Further I think > > > "size less than or equal to the size requested" > > > is quite ambiguous in the calloc case, isn't the size requested in the > > > calloc case actually nmemb * size rather than just size? > > > > This is unclear but it can be understood this way. > > This was also Joseph's point. > > > > I am happy to submit a patch that changes the code so > > that the swapped arguments to calloc do not cause a warning > > anymore. > > That would be my preference because then the allocation size is > correct and it is purely a style warning. > It doesn't follow how the warning is described: > "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" > when the size is certainly sufficient. The C standard defines the semantics of to allocate space of 'nmemb' objects of size 'size', so I would say the warning and its description are correct because if you call calloc with '1' as size argument but the object size is larger then you specify an insufficient size for the object given the semantical description of calloc in the standard. If this does not affect alignment, then this should not matter, but it is still not really correct. > > But wonder what others think about it. > > BTW, shouldn't the warning be for C++ as well? Sure, I know, > people use operator new more often, but still, the > allocators are used in there as well. We should, but it I am not familiar with the C++ FE. > > We have the -Wmemset-transposed-args warning, couldn't we > have a similar one for calloc, and perhaps do it solely in > the case where one uses sizeof of the type used in the cast > pointer? > So warn for > (struct S *) calloc (sizeof (struct S), 1) > or > (struct S *) calloc (sizeof (struct S), n) > but not for > (struct S *) calloc (4, 15) > or > (struct S *) calloc (sizeof (struct T), 1) > or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. > > Jakub Yes, although in contrast to -Wmeset-transposed-args this would be considered a "style" option which then nobody would activate. And if we put it into -Wextra then we have the same situation as today. Martin
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Wed, Dec 06, 2023 at 03:56:10PM +0100, Martin Uecker wrote: > > That would be my preference because then the allocation size is > > correct and it is purely a style warning. > > It doesn't follow how the warning is described: > > "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" > > when the size is certainly sufficient. > > The C standard defines the semantics of to allocate space > of 'nmemb' objects of size 'size', so I would say > the warning and its description are correct because > if you call calloc with '1' as size argument but > the object size is larger then you specify an > insufficient size for the object given the semantical > description of calloc in the standard. 1 is sizeof (char), so you ask for an array of sizeof (struct ...) chars and store the struct into it. > > We have the -Wmemset-transposed-args warning, couldn't we > > have a similar one for calloc, and perhaps do it solely in > > the case where one uses sizeof of the type used in the cast > > pointer? > > So warn for > > (struct S *) calloc (sizeof (struct S), 1) > > or > > (struct S *) calloc (sizeof (struct S), n) > > but not for > > (struct S *) calloc (4, 15) > > or > > (struct S *) calloc (sizeof (struct T), 1) > > or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. > > Yes, although in contrast to -Wmeset-transposed-args > this would be considered a "style" option which then > nobody would activate. And if we put it into -Wextra > then we have the same situation as today. Well, the significant difference would be that users would know that they got the size for the allocation right, just that a coding style says it is better to put the type's size as the second argument rather than first, and they could disable that warning separately from -Walloc-size and still get warnings on (struct S *) calloc (1, 1) or (struct S *) malloc (3) if sizeof (struct S) is 24... Jakub
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Am Mittwoch, dem 06.12.2023 um 16:01 +0100 schrieb Jakub Jelinek: > On Wed, Dec 06, 2023 at 03:56:10PM +0100, Martin Uecker wrote: > > > That would be my preference because then the allocation size is > > > correct and it is purely a style warning. > > > It doesn't follow how the warning is described: > > > "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" > > > when the size is certainly sufficient. > > > > The C standard defines the semantics of to allocate space > > of 'nmemb' objects of size 'size', so I would say > > the warning and its description are correct because > > if you call calloc with '1' as size argument but > > the object size is larger then you specify an > > insufficient size for the object given the semantical > > description of calloc in the standard. > > 1 is sizeof (char), so you ask for an array of sizeof (struct ...) > chars and store the struct into it. If you use char *p = calloc(sizeof(struct foo), 1); it does not warn. > > > > We have the -Wmemset-transposed-args warning, couldn't we > > > have a similar one for calloc, and perhaps do it solely in > > > the case where one uses sizeof of the type used in the cast > > > pointer? > > > So warn for > > > (struct S *) calloc (sizeof (struct S), 1) > > > or > > > (struct S *) calloc (sizeof (struct S), n) > > > but not for > > > (struct S *) calloc (4, 15) > > > or > > > (struct S *) calloc (sizeof (struct T), 1) > > > or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. > > > > Yes, although in contrast to -Wmeset-transposed-args > > this would be considered a "style" option which then > > nobody would activate. And if we put it into -Wextra > > then we have the same situation as today. > > Well, the significant difference would be that users would > know that they got the size for the allocation right, just > that a coding style says it is better to put the type's size > as the second argument rather than first, and they could disable > that warning separately from -Walloc-size and still get warnings > on (struct S *) calloc (1, 1) or (struct S *) malloc (3) if > sizeof (struct S) is 24... Ok. Note that another limitation of the current version is that it does not warn for ... = (struct S*) calloc (...) with the cast (which is non-idiomatic in C). This is also something I would like to address in the future and would be more important for the C++ version. But for this case it should probably use the type of the cast and the warning needs to be added somewhere else in the FE. Martin
Re: [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Wed, Dec 6, 2023 at 10:13 AM Martin Uecker wrote: > > Am Mittwoch, dem 06.12.2023 um 16:01 +0100 schrieb Jakub Jelinek: > > On Wed, Dec 06, 2023 at 03:56:10PM +0100, Martin Uecker wrote: > > > > That would be my preference because then the allocation size is > > > > correct and it is purely a style warning. > > > > It doesn't follow how the warning is described: > > > > "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" > > > > when the size is certainly sufficient. > > > > > > The C standard defines the semantics of to allocate space > > > of 'nmemb' objects of size 'size', so I would say > > > the warning and its description are correct because > > > if you call calloc with '1' as size argument but > > > the object size is larger then you specify an > > > insufficient size for the object given the semantical > > > description of calloc in the standard. > > > > 1 is sizeof (char), so you ask for an array of sizeof (struct ...) > > chars and store the struct into it. > > If you use > > char *p = calloc(sizeof(struct foo), 1); > > it does not warn. > > > > > > > We have the -Wmemset-transposed-args warning, couldn't we > > > > have a similar one for calloc, and perhaps do it solely in > > > > the case where one uses sizeof of the type used in the cast > > > > pointer? > > > > So warn for > > > > (struct S *) calloc (sizeof (struct S), 1) > > > > or > > > > (struct S *) calloc (sizeof (struct S), n) > > > > but not for > > > > (struct S *) calloc (4, 15) > > > > or > > > > (struct S *) calloc (sizeof (struct T), 1) > > > > or similar? Of course check for compatible types of TYPE_MAIN_VARIANTs. > > > > > > Yes, although in contrast to -Wmeset-transposed-args > > > this would be considered a "style" option which then > > > nobody would activate. And if we put it into -Wextra > > > then we have the same situation as today. > > > > Well, the significant difference would be that users would > > know that they got the size for the allocation right, just > > that a coding style says it is better to put the type's size > > as the second argument rather than first, and they could disable > > that warning separately from -Walloc-size and still get warnings > > on (struct S *) calloc (1, 1) or (struct S *) malloc (3) if > > sizeof (struct S) is 24... > > Ok. > > Note that another limitation of the current version is that it > does not warn for > > ... = (struct S*) calloc (...) > > with the cast (which is non-idiomatic in C). Note that -Wc++-compat encourages the cast, for people who are trying to make their code compilable as both C and C++. > This is also > something I would like to address in the future and would be > more important for the C++ version. But for this case it > should probably use the type of the cast and the warning > needs to be added somewhere else in the FE. > > > Martin >
Re: [PING] [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Tue, 31 Oct 2023, Martin Uecker wrote: > > + if (TREE_CODE (arg) == INTEGER_CST > > + && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl))) What if TYPE_SIZE_UNIT (ttl) is not an INTEGER_CST? I don't see any tests of the case of assigning to a pointer to a variably sized type. -- Joseph S. Myers jos...@codesourcery.com
Re: [PING] [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
Am Dienstag, dem 31.10.2023 um 22:19 + schrieb Joseph Myers: > On Tue, 31 Oct 2023, Martin Uecker wrote: > > > > + if (TREE_CODE (arg) == INTEGER_CST > > > + && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl))) > > What if TYPE_SIZE_UNIT (ttl) is not an INTEGER_CST? I don't see any tests > of the case of assigning to a pointer to a variably sized type. > Right. Thanks! Revised version attached. Martin c: Add Walloc-size to warn about insufficient size in allocations [PR71219] Add option Walloc-size that warns about allocations that have insufficient storage for the target type of the pointer the storage is assigned to. Added to Wextra. PR c/71219 gcc: * doc/invoke.texi: Document -Walloc-size option. gcc/c-family: * c.opt (Walloc-size): New option. gcc/c: * c-typeck.cc (convert_for_assignment): Add warning. gcc/testsuite: * gcc.dg/Walloc-size-1.c: New test. * gcc.dg/Walloc-size-2.c: New test. diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 44b9c862c14..29d3d789a49 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -331,6 +331,10 @@ Walloca C ObjC C++ ObjC++ Var(warn_alloca) Warning Warn on any use of alloca. +Walloc-size +C ObjC Var(warn_alloc_size) Warning LangEnabledBy(C ObjC, Wextra) +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= Warn for calls to allocation functions that diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index 0de4662bfc6..16fadfb5468 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -7347,6 +7347,34 @@ 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 that are not big enough for the target +type. */ + tree fndecl; + if (warn_alloc_size + && 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 + && INTEGER_CST == TREE_CODE (TYPE_SIZE_UNIT (ttl)) + && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl))) +warning_at (location, OPT_Walloc_size, "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 5a9284d635c..815a33d4b87 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -8138,6 +8138,16 @@ 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-size +@opindex Walloc-size +@item -Walloc-size +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-size-1.c b/gcc/testsuite/gcc.dg/Walloc-size-1.c new file mode 100644 index 000..61806f58192 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Walloc-size-1.c @@ -0,0 +1,36 @@ +/* Tests the warnings for insufficient allocation size. + { dg-do compile } + { dg-options "-Walloc-size" } + * */ +#include +#include + +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
Re: [PING] [C PATCH, v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]
On Wed, 1 Nov 2023, Martin Uecker wrote: > Am Dienstag, dem 31.10.2023 um 22:19 + schrieb Joseph Myers: > > On Tue, 31 Oct 2023, Martin Uecker wrote: > > > > > > + if (TREE_CODE (arg) == INTEGER_CST > > > > + && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl))) > > > > What if TYPE_SIZE_UNIT (ttl) is not an INTEGER_CST? I don't see any tests > > of the case of assigning to a pointer to a variably sized type. > > > > Right. Thanks! Revised version attached. This version is OK. -- Joseph S. Myers jos...@codesourcery.com