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

[Jakub Jelinek <jakub@redhat.com>]

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