Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Jakub Jelinek]

On Fri, Dec 09, 2016 at 10:44:17AM -0700, Martin Sebor wrote:
> On 12/09/2016 06:26 AM, Andreas Schwab wrote:
> >FAIL: gcc.dg/attr-alloc_size-3.c  (test for warnings, line 445)
> >FAIL: gcc.dg/attr-alloc_size-3.c (test for excess errors)
> >Excess errors:
> >/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:244:3:
> > warning: product '65535 * 65535' of arguments 1 and 2 exceeds maximum 
> >object size 2147483647 [-Walloc-size-larger-than=]
> >/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:298:3:
> > warning: argument 1 value '4294967294' exceeds maximum object size 
> >2147483647 [-Walloc-size-larger-than=]
> >/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:299:3:
> > warning: argument 1 value '4294967295' exceeds maximum object size 
> >2147483647 [-Walloc-size-larger-than=]
> 
> I assume this was on an ILP32 target like i686.  It should be fixed
> by r243497 along with the failures on arm* targets that Christophe
> pointed out earlier.  Please let me know if that doesn't clear it
> up for you (and what target).

on arm-unknown-linux-gnueabi (and likely other ILP32) targets.
doesn't look like valid ChangeLog entry (r243497).
Also, especially when adding new tests related to gimple-ssa-sprintf.c
that keep failing on non-x86_64 a lot, it would be nice if you at least
tested them through
make check-gcc RUNTESTFLAGS="--target_board=unix\{-m32,-m64\} 
dg.exp=name_of_test*"
first.

Jakub

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

On 12/09/2016 06:26 AM, Andreas Schwab wrote:

FAIL: gcc.dg/attr-alloc_size-3.c  (test for warnings, line 445)
FAIL: gcc.dg/attr-alloc_size-3.c (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:244:3: 
warning: product '65535 * 65535' of arguments 1 and 2 exceeds maximum object 
size 2147483647 [-Walloc-size-larger-than=]
/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:298:3: 
warning: argument 1 value '4294967294' exceeds maximum object size 2147483647 
[-Walloc-size-larger-than=]
/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:299:3: 
warning: argument 1 value '4294967295' exceeds maximum object size 2147483647 
[-Walloc-size-larger-than=]


I assume this was on an ILP32 target like i686.  It should be fixed
by r243497 along with the failures on arm* targets that Christophe
pointed out earlier.  Please let me know if that doesn't clear it
up for you (and what target).

Thanks
Martin

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

On 12/09/2016 02:59 AM, Christophe Lyon wrote:

On 9 December 2016 at 00:54, Martin Sebor  wrote:

+enabled with @option{-Wextra}.


So I think we should in the immediate term not enable this in Wextra.
However, I think for gcc-8 we should revisit after fixing GCC to be
cleaner WRT alloc-zero.

So disable alloc-zero by default, comment typo and potentially adding
the GTY marker to alloc_object_size_limit and this is OK.



Thanks.  Committed in r243470 with the changes above.

Martin


Hi,

The new test attr-alloc_size-3.c fails on arm* targets:
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:
In function 'test_schar_cst':
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:175:3:
warning: argument 1 value '-128' is negative
[-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:42:7:
note: in a call to allocation function 'f_schar_1' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:176:3:
warning: argument 1 value '-1' is negative [-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:42:7:
note: in a call to allocation function 'f_schar_1' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:181:3:
warning: argument 2 value '-128' is negative
[-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:43:7:
note: in a call to allocation function 'f_schar_2' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:182:3:
warning: argument 1 value '-128' is negative
[-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:183:3:
warning: argument 2 value '-1' is negative [-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:43:7:
note: in a call to allocation function 'f_schar_2' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:184:3:
warning: argument 1 value '-1' is negative [-Walloc-size-larger-than=]


Thanks for the heads up!  I've tweaked the test to avoid these
failures (tested with a arm-unknown-linux-gnueabi cross-compiler)
and committed r243497.

Martin

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Andreas Schwab]

FAIL: gcc.dg/attr-alloc_size-3.c  (test for warnings, line 445)
FAIL: gcc.dg/attr-alloc_size-3.c (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:244:3: 
warning: product '65535 * 65535' of arguments 1 and 2 exceeds maximum object 
size 2147483647 [-Walloc-size-larger-than=]
/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:298:3: 
warning: argument 1 value '4294967294' exceeds maximum object size 2147483647 
[-Walloc-size-larger-than=]
/daten/aranym/gcc/gcc-20161209/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:299:3: 
warning: argument 1 value '4294967295' exceeds maximum object size 2147483647 
[-Walloc-size-larger-than=]

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Christophe Lyon]

On 9 December 2016 at 00:54, Martin Sebor  wrote:
>>> +enabled with @option{-Wextra}.
>>
>> So I think we should in the immediate term not enable this in Wextra.
>> However, I think for gcc-8 we should revisit after fixing GCC to be
>> cleaner WRT alloc-zero.
>>
>> So disable alloc-zero by default, comment typo and potentially adding
>> the GTY marker to alloc_object_size_limit and this is OK.
>
>
> Thanks.  Committed in r243470 with the changes above.
>
> Martin

Hi,

The new test attr-alloc_size-3.c fails on arm* targets:
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:
In function 'test_schar_cst':
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:175:3:
warning: argument 1 value '-128' is negative
[-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:42:7:
note: in a call to allocation function 'f_schar_1' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:176:3:
warning: argument 1 value '-1' is negative [-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:42:7:
note: in a call to allocation function 'f_schar_1' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:181:3:
warning: argument 2 value '-128' is negative
[-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:43:7:
note: in a call to allocation function 'f_schar_2' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:182:3:
warning: argument 1 value '-128' is negative
[-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:183:3:
warning: argument 2 value '-1' is negative [-Walloc-size-larger-than=]
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:43:7:
note: in a call to allocation function 'f_schar_2' declared here
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.dg/attr-alloc_size-3.c:184:3:
warning: argument 1 value '-1' is negative [-Walloc-size-larger-than=]

[]

Christophe

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

+enabled with @option{-Wextra}.

So I think we should in the immediate term not enable this in Wextra.
However, I think for gcc-8 we should revisit after fixing GCC to be
cleaner WRT alloc-zero.

So disable alloc-zero by default, comment typo and potentially adding
the GTY marker to alloc_object_size_limit and this is OK.


Thanks.  Committed in r243470 with the changes above.

Martin

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Jeff Law]

On 11/19/2016 05:55 PM, Martin Sebor wrote:



gcc-78284.diff


PR c/77531 - __attribute__((alloc_size(1,2))) could also warn on multiplication 
overflow
PR c/78284 - warn on malloc with very large arguments

gcc/c-family/ChangeLog:

PR c/77531
PR c/78284
* c.opt (-Walloc-zero, -Walloc-size-larger-than): New options.

gcc/ChangeLog:

PR c/77531
PR c/78284
* builtin-attrs.def (ATTR_ALLOC_SIZE, ATTR_RETURNS_NONNULL): New
identifier tree nodes.
(ATTR_ALLOCA_SIZE_1_NOTHROW_LEAF_LIST): New attribute list.
(ATTR_MALLOC_SIZE_1_NOTHROW_LIST): Same.
(ATTR_MALLOC_SIZE_1_NOTHROW_LEAF_LIST): Same.
(ATTR_MALLOC_SIZE_1_2_NOTHROW_LEAF_LIST): Same.
(ATTR_ALLOC_SIZE_2_NOTHROW_LEAF_LIST): Same.
* builtins.c (expand_builtin_alloca): Call
maybe_warn_alloc_args_overflow.
* builtins.def (aligned_alloc, calloc, malloc, realloc):
Add attribute alloc_size.
(alloca): Add attribute alloc_size and returns_nonnull.
* calls.h (maybe_warn_alloc_args_overflow): Declare.
* calls.c (alloc_max_size, operand_signed_p): New functions.
(maybe_warn_alloc_args_overflow): Define.
(initialize_argument_information): Diagnose overflow in functions
declared with attaribute alloc_size.
* doc/invoke.texi (Warning Options): Document -Walloc-zero and
-Walloc-size-larger-than.

gcc/testsuite/ChangeLog:

PR c/77531
PR c/78284
* gcc.dg/attr-alloc_size-3.c: New test.
* gcc.dg/attr-alloc_size-4.c: New test.
* gcc.dg/attr-alloc_size-5.c: New test.
* gcc.dg/attr-alloc_size-6.c: New test.
* gcc.dg/attr-alloc_size-7.c: New test.
* gcc.dg/attr-alloc_size-8.c: New test.
* gcc.dg/attr-alloc_size-9.c: New test.
* gcc/testsuite/gcc.dg/errno-1.c: Adjust.




diff --git a/gcc/calls.c b/gcc/calls.c
index c916e07..05e6e09 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1181,6 +1183,310 @@ store_unaligned_arguments_into_pseudos (struct arg_data 
*args, int num_actuals)
   }
 }

+static tree alloc_object_size_limit;
Is this object live across GCC passes (I think it is)?  If so, then it 
needs a GTY marker.







+ /* G++ emits calls to ::operator new[](SIZE_MAX) in C++98
+mode and with -fno-exceptions as a way to indicate array
+size overflow.  There's no good way to detect C++98 here
+so avoid diagnosing these calls for all C++ modes.  */

How unfortunate :(



+ else if (range_type == VR_ANTI_RANGE)
+   {
+ /* For an anti-range, if the type of the formal argument
+is unsigned and the bounds of the range are of opposite
+signs when interpreted as signed, check to see if the
+type of the actual argument is signed.  If so, the lower
+bound must be taken to be zero (rather than a large
+positive value corresonding to the actual lower bound
+interpreted as unsigned) and there is nothing else that
+can be inferred from it.  */

s/corresonding/corresponding/







+@item -Walloc-zero
+@opindex Wno-alloc-zero
+@opindex Walloc-zero
+Warn about calls to allocation functions decorated with attribute
+@code{alloc_size} that specify zero bytes, including those to the built-in
+forms of the functions @code{aligned_alloc}, @code{alloca}, @code{calloc},
+@code{malloc}, and @code{realloc}.  Because the behavior of these functions
+when called with a zero size differs among implementations relying on it may
+result in subtle portability bugs and should be avoided.  This option is
+enabled with @option{-Wextra}.
So I think we should in the immediate term not enable this in Wextra. 
However, I think for gcc-8 we should revisit after fixing GCC to be 
cleaner WRT alloc-zero.


So disable alloc-zero by default, comment typo and potentially adding 
the GTY marker to alloc_object_size_limit and this is OK.


Jeff

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Jeff Law]

On 11/19/2016 05:55 PM, Martin Sebor wrote:

The attached update is an attempt to incorporate the feedback
I received last week during the discussion of the prerequisite
patch to avoid calling alloca(0)​.

The important changes are:

1) Add attribute returns_nonnull to __builtin_alloca.
This should be broken out into its own change.  I can't see a good 
reason offhand why this has to be tied up with the overflow checking.


I'll go ahead and pre-approve this part. (builtin-attrs.def/builtins.def 
changes).  If they can be installed separately, please do.


I won't have the time to look at the rest of this today.

jeff

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

On 11/20/2016 04:56 AM, Bernd Edlinger wrote:

Hi,

when you add a returns_nonnull to the builtin alloca
then this code in tree-vrp.c (gimple_stmt_nonzero_warnv_p)
should go away:

 if (flag_delete_null_pointer_checks &&
 lookup_attribute ("returns_nonnull",
   TYPE_ATTRIBUTES (gimple_call_fntype (stmt
   return true;
 return gimple_alloca_call_p (stmt);


Yes, that looks like it should be superfluous now.  Let me test
it.  (It will make the null pointer test removal conditional on
flag_delete_null_pointer_checks but presumably that's okay.)



Regarding __builtin_alloca_with_align, I see no test cases
for that function:

__builtin_alloca_with_align is a bit special, because it allocates
more than the first parameter says, I think it allocates (if size != 0)
size + align/8 in order to be able to return an aligned object.
What if align is very large?


I'm guessing there are only a handful of tests for
__builtin_alloca_with_align because the built-in is used to allocate
VLAs and so it's exercised indirectly by VLA tests.  There is one
fairly comprehensive test for the built-in that exercises the case
you mention: builtins-68.c, though perhaps not to the extent it
could or should.  The problem is that there is no mechanism for
a compiler to indicate that a VLA has failed to allocate.  So while
the following compiles (with my patch with a warning), it crashes
at runtime:

$ cat x.c && gcc -O2 -S x.c
void sink (void*);

void g (void)
{
  struct S { int _Alignas (1024) i; };

  unsigned long N = __SIZE_MAX__ / _Alignof (struct S);

  struct S vla [N];
  sink (vla);
}


x.c: In function ‘g’:
x.c:9:12: warning: argument 1 value ‘18446744073709550592ul’ exceeds 
maximum object size 9223372036854775807 [-Walloc-size-larger-than=]

   struct S vla [N];
^~~
x.c:9:12: note: in a call to built-in allocation function 
‘__builtin_alloca_with_align’


Or, with -Wvla-larger-than=1000 (or any other positive number):

x.c:9:12: warning: argument to variable-length array is too large 
[-Wvla-larger-than=]

   struct S vla [N];
^~~
x.c:9:12: note: limit is 1000 bytes, but argument is 18446744073709550592


I still would prefer separate -walloc-zero and -Walloca-zero
options.  Likewise for the -Walloc-larger-than and -Walloca-larger-than
or better -Wmalloc-larger-than and -Walloca-larger-than ?


Sure, I'm open to it.  But before I invest time into tweaking
things further I would like to get consensus on the two patches
I've posted (alloca(0) and this one), and ideally get them
at least tentatively approved.  Otherwise, I would worry it
might be a wasted effort.

Martin

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Bernd Edlinger]

Hi,

when you add a returns_nonnull to the builtin alloca
then this code in tree-vrp.c (gimple_stmt_nonzero_warnv_p)
should go away:

 if (flag_delete_null_pointer_checks &&
 lookup_attribute ("returns_nonnull",
   TYPE_ATTRIBUTES (gimple_call_fntype (stmt
   return true;
 return gimple_alloca_call_p (stmt);


Regarding __builtin_alloca_with_align, I see no test cases
for that function:

__builtin_alloca_with_align is a bit special, because it allocates
more than the first parameter says, I think it allocates (if size != 0)
size + align/8 in order to be able to return an aligned object.
What if align is very large?

I still would prefer separate -walloc-zero and -Walloca-zero
options.  Likewise for the -Walloc-larger-than and -Walloca-larger-than
or better -Wmalloc-larger-than and -Walloca-larger-than ?


Bernd.

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

The attached update is an attempt to incorporate the feedback
I received last week during the discussion of the prerequisite
patch to avoid calling alloca(0)​.

The important changes are:

1) Add attribute returns_nonnull to __builtin_alloca.
2) Prevent calls to alloca(0) from triggering the -Walloc-zero
   warning when the call is to the function named alloca (as
   opposed to one made explicitly to __builtin_alloca).  I don't
   think this special treatment is necessary but a concern was
   voiced that short of disabling the warning altogether there
   was no way to suppress the warning for individual calls to
   alloca without adding 1 to the size, and that the addition
   might adversely affect performance.
3) Fix a bug in the handling of certain calls to calloc(a, b)
   with signed arguments.

Besides bootstrapping GCC and running the test suite I tested
this patch by compiling Binutils 2.27, Busybox 1.25.1, the trunk
of Glibc, and the Linux kernel with no instances of either of
the new warnings.  (GCC requires the alloca(0) patch to fix
the -Walloc-zero warnings.)

Since there has been quite a bit of discussion in response to
the related and prerequisite alloca(0) patch for GCC let me
summarize the rationale for this patch and the two new warnings
it adds:

1) -Walloc-larger-than=max (enabled by default with SIZE_MAX / 2)
   Calls to calloc(a, b) where the product (a * b) overflows are
   a source of bugs.  calloc is expected to fail in this case but
   implementations have been known to get this wrong.  See [1].

   Calls to allocation functions such as malloc with a very large
   argument can be the result of an incorrect computation or
   conversion from a negative argument, or integer overflow.
   Such calls normally fail at runtime but malloc error handling
   is often poorly exercised and the error handling code buggy.
   Detecting these early helps avoid these problems.

2) -Walloc-zero (enabled with -Wextra)
   Zero allocations are a notorious source bugs and security
   vulnerabilities, not only because the result of such calls
   is implementation-defined (some return null, others a non-null
   pointer to a zero-sized object), but also because such calls
   are sometimes the result of integer overflow.  See [2].
   In addition, C11 has deprecated calling realloc(0, p) in
   response to defect report 400 [3].
   Finally, calling alloca(0) is dangerous because like malloc(0),
   the result may be either null or non-null, but unlike malloc,
   when non-null, the pointer need not be distinct from others.
   This warning is analogous to the Clang static analyzer warning
   for zero-size allocation calls.

[1] RUS-CERT Advisory 2002-08:02 – Flaw in calloc and similar
routines
https://cert.uni-stuttgart.de/ticker/advisories/calloc.html

[2] Zero-sized heap allocations vulnerability analysis
https://www.usenix.org/legacy/event/woot10/tech/full_papers/Vanegue.pdf

[3] DR 400 - realloc with size zero problems
http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_400

On 11/17/2016 05:15 PM, Martin Sebor wrote:

Attached is an update to the patch that avoids duplicating the
-Walloca-larger-than warnings.  This version also avoids warning
for calls with zero allocation size to functions declared with
the returns_nonnull attribute (like libiberty's xmalloc).  Since
such functions cannot return null there's no portability issue
to worry/warn about.

When applied along with the patch to avoid calling alloca(0)
in GCC posted earlier today (link below) this version bootstraps
and passes all tests on x86_64.  It also builds Binutils 2.27 and
the Linux kernel with no new warnings.

  https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01838.html

Martin

On 11/16/2016 10:19 AM, Martin Sebor wrote:

Attached is an updated version of the patch that also adds attribute
alloc_size to the standard allocation built-ins (aligned_alloc,
alloca, malloc, calloc, and realloc) and handles alloca.

Besides that, I've renamed the option to -Walloc-size-larger-than
to make it less similar to -Walloca-larger-than.  It think the new
name works because the option works with the alloc_size attribute.
 Other suggestions are of course welcome.

I've left the alloc_max_size function in place until I receive some
feedback on it.

I've regression-tested the patch on x86_64 with a few issues.  The
biggest is that the -Walloc-zero option enabled by -Wextra causes
a number of errors during bootstrap due to invoking the XALLOCAVEC
macro with a zero argument.  The errors look valid to me (and I
got past them by temporarily changing the XALLOCAVEC macro to
always allocate at least one byte) but I haven't fixed the errors
yet.  I'll post a separate patch for those.   The other open issue
is that the new warning duplicates a small subset of the
-Walloca-larger-than warnings.  I expect removing the duplicates
to be straightforward.  I post this updated patch for review while
I work on the remaining issues.

Martin

On 11/13/2016 08:19 

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

Attached is an update to the patch that avoids duplicating the
-Walloca-larger-than warnings.  This version also avoids warning
for calls with zero allocation size to functions declared with
the returns_nonnull attribute (like libiberty's xmalloc).  Since
such functions cannot return null there's no portability issue
to worry/warn about.

When applied along with the patch to avoid calling alloca(0)
in GCC posted earlier today (link below) this version bootstraps
and passes all tests on x86_64.  It also builds Binutils 2.27 and
the Linux kernel with no new warnings.

  https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01838.html

Martin

On 11/16/2016 10:19 AM, Martin Sebor wrote:

Attached is an updated version of the patch that also adds attribute
alloc_size to the standard allocation built-ins (aligned_alloc,
alloca, malloc, calloc, and realloc) and handles alloca.

Besides that, I've renamed the option to -Walloc-size-larger-than
to make it less similar to -Walloca-larger-than.  It think the new
name works because the option works with the alloc_size attribute.
 Other suggestions are of course welcome.

I've left the alloc_max_size function in place until I receive some
feedback on it.

I've regression-tested the patch on x86_64 with a few issues.  The
biggest is that the -Walloc-zero option enabled by -Wextra causes
a number of errors during bootstrap due to invoking the XALLOCAVEC
macro with a zero argument.  The errors look valid to me (and I
got past them by temporarily changing the XALLOCAVEC macro to
always allocate at least one byte) but I haven't fixed the errors
yet.  I'll post a separate patch for those.   The other open issue
is that the new warning duplicates a small subset of the
-Walloca-larger-than warnings.  I expect removing the duplicates
to be straightforward.  I post this updated patch for review while
I work on the remaining issues.

Martin

On 11/13/2016 08:19 PM, Martin Sebor wrote:

Bug 77531 requests a new warning for calls to allocation functions
(those declared with attribute alloc_size(X, Y)) that overflow the
computation X * Z of the size of the allocated object.

Bug 78284 suggests that detecting and diagnosing other common errors
in calls to allocation functions, such as allocating more space than
SIZE_MAX / 2 bytes, would help prevent subsequent buffer overflows.

The attached patch adds two new warning options, -Walloc-zero and
-Walloc-larger-than=bytes that implement these two enhancements.
The patch is not 100% finished because, as it turns out, the GCC
allocation built-ins (malloc et al.) do not make use of the
attribute and so don't benefit from the warnings.  The tests are
also incomplete, and there's at least one bug in the implementation
I know about.

I'm posting the patch while stage 1 is still open and to give
a heads up on it and to get early feedback.  I expect completing
it will be straightforward.

Martin

PS The alloc_max_size function added in the patch handles sizes
specified using suffixes like KB, MB, etc.  I added that to make
it possible to specify sizes in excess of the maximum of INT_MAX
that (AFAIK) options that take integer arguments handle out of
the box.  It only belatedly occurred to me that the suffixes
are unnecessary if the option argument is handled using strtoull.
I can remove the suffix (as I suspect it will raise objections)
but I think that a general solution along these lines would be
useful to let users specify large byte sizes in other options
as well (such -Walloca-larger-than, -Wvla-larger-then).  Are
there any suggestions or preferences?





PR c/77531 - __attribute__((alloc_size(1,2))) could also warn on multiplication overflow
PR c/78284 - warn on malloc with very large arguments

gcc/c-family/ChangeLog:

	PR c/77531
	PR c/78284
	* c.opt (-Walloc-zero, -Walloc-size-larger-than): New options.

gcc/ChangeLog:

	PR c/77531
	PR c/78284
	* builtin-attrs.def (ATTR_ALLOC_SIZE): New identifier tree.
	(ATTR_MALLOC_SIZE_1_NOTHROW_LIST): New attribute list.
	(ATTR_MALLOC_SIZE_1_NOTHROW_LEAF_LIST): Same.
	(ATTR_MALLOC_SIZE_1_2_NOTHROW_LEAF_LIST): Same.
	(ATTR_ALLOC_SIZE_2_NOTHROW_LEAF_LIST): Same.
	* builtins.c (expand_builtin_alloca): Call
	maybe_warn_alloc_args_overflow.
	* builtins.def (akigned_alloc, alloca, calloc, malloc, realloc):
	Add attribute alloc_size.
	* calls.h (maybe_warn_alloc_args_overflow): Declare.
	* calls.c (alloc_max_size): New function.
	(maybe_warn_alloc_args_overflow): Define.
	(initialize_argument_information): Diagnose overflow in functions
	declared with attaribute alloc_size.
	* doc/invoke.texi (Warning Options): Document -Walloc-zero and
	-Walloc-size-larger-than.

gcc/testsuite/ChangeLog:

	PR c/77531
	PR c/78284
	* gcc.dg/attr-alloc_size-3.c: New test.
	* gcc.dg/attr-alloc_size-4.c: New test.
	* gcc.dg/attr-alloc_size-5.c: New test.
	* gcc.dg/attr-alloc_size-6.c: New test.
	* gcc.dg/attr-alloc_size-7.c: New test.
	* gcc.dg/attr-alloc_size-8.c: New test.
	* gcc.dg/attr-alloc_size-9.c: New test.
	* 

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

Attached is an updated version of the patch that also adds attribute
alloc_size to the standard allocation built-ins (aligned_alloc,
alloca, malloc, calloc, and realloc) and handles alloca.

Besides that, I've renamed the option to -Walloc-size-larger-than
to make it less similar to -Walloca-larger-than.  It think the new
name works because the option works with the alloc_size attribute.
 Other suggestions are of course welcome.

I've left the alloc_max_size function in place until I receive some
feedback on it.

I've regression-tested the patch on x86_64 with a few issues.  The
biggest is that the -Walloc-zero option enabled by -Wextra causes
a number of errors during bootstrap due to invoking the XALLOCAVEC
macro with a zero argument.  The errors look valid to me (and I
got past them by temporarily changing the XALLOCAVEC macro to
always allocate at least one byte) but I haven't fixed the errors
yet.  I'll post a separate patch for those.   The other open issue
is that the new warning duplicates a small subset of the
-Walloca-larger-than warnings.  I expect removing the duplicates
to be straightforward.  I post this updated patch for review while
I work on the remaining issues.

Martin

On 11/13/2016 08:19 PM, Martin Sebor wrote:

Bug 77531 requests a new warning for calls to allocation functions
(those declared with attribute alloc_size(X, Y)) that overflow the
computation X * Z of the size of the allocated object.

Bug 78284 suggests that detecting and diagnosing other common errors
in calls to allocation functions, such as allocating more space than
SIZE_MAX / 2 bytes, would help prevent subsequent buffer overflows.

The attached patch adds two new warning options, -Walloc-zero and
-Walloc-larger-than=bytes that implement these two enhancements.
The patch is not 100% finished because, as it turns out, the GCC
allocation built-ins (malloc et al.) do not make use of the
attribute and so don't benefit from the warnings.  The tests are
also incomplete, and there's at least one bug in the implementation
I know about.

I'm posting the patch while stage 1 is still open and to give
a heads up on it and to get early feedback.  I expect completing
it will be straightforward.

Martin

PS The alloc_max_size function added in the patch handles sizes
specified using suffixes like KB, MB, etc.  I added that to make
it possible to specify sizes in excess of the maximum of INT_MAX
that (AFAIK) options that take integer arguments handle out of
the box.  It only belatedly occurred to me that the suffixes
are unnecessary if the option argument is handled using strtoull.
I can remove the suffix (as I suspect it will raise objections)
but I think that a general solution along these lines would be
useful to let users specify large byte sizes in other options
as well (such -Walloca-larger-than, -Wvla-larger-then).  Are
there any suggestions or preferences?



PR c/77531 - __attribute__((alloc_size(1,2))) could also warn on multiplication overflow
PR c/78284 - warn on malloc with very large arguments

include/ChangeLog:
	* libiberty.h (XALLOCAVEC): Make sure alloca argument is non-zero.

gcc/c-family/ChangeLog:

	PR c/77531
	PR c/78284
	* c.opt (-Walloc-zero, -Walloc-larger-than): New options.

gcc/ChangeLog:

	PR c/77531
	PR c/78284
	* builtin-attrs.def (ATTR_ALLOC_SIZE): New identifier tree.
	(ATTR_MALLOC_SIZE_1_NOTHROW_LIST): New attribute list.
	(ATTR_MALLOC_SIZE_1_NOTHROW_LEAF_LIST): Same.
	(ATTR_MALLOC_SIZE_1_2_NOTHROW_LEAF_LIST): Same.
	(ATTR_ALLOC_SIZE_2_NOTHROW_LEAF_LIST): Same.
	* builtins.c (expand_builtin_alloca): Call
	maybe_warn_alloc_args_overflow.
	* builtins.def (akigned_alloc, alloca, calloc, malloc, realloc):
	Add attribute alloc_size.
	* calls.h (maybe_warn_alloc_args_overflow): Declare.
	* calls.c (alloc_max_size): New function.
	(maybe_warn_alloc_args_overflow): Define.
	(initialize_argument_information): Diagnose overflow in functions
	declared with attaribute alloc_size.
	* doc/invoke.texi (Warning Options): Document -Walloc-zero and
	-Walloc-larger-than.

gcc/testsuite/ChangeLog:

	PR c/77531
	PR c/78284
	* gcc.dg/attr-alloc_size-3.c: New test.
	* gcc.dg/attr-alloc_size-4.c: New test.
	* gcc.dg/attr-alloc_size-5.c: New test.
	* gcc.dg/attr-alloc_size-6.c: New test.
	* gcc.dg/attr-alloc_size-7.c: New test.

diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def
index 8dc59c9..2a58b31 100644
--- a/gcc/builtin-attrs.def
+++ b/gcc/builtin-attrs.def
@@ -83,6 +83,7 @@ DEF_LIST_INT_INT (5,6)
 #undef DEF_LIST_INT_INT
 
 /* Construct trees for identifiers.  */
+DEF_ATTR_IDENT (ATTR_ALLOC_SIZE, "alloc_size")
 DEF_ATTR_IDENT (ATTR_COLD, "cold")
 DEF_ATTR_IDENT (ATTR_CONST, "const")
 DEF_ATTR_IDENT (ATTR_FORMAT, "format")
@@ -150,6 +151,23 @@ DEF_ATTR_TREE_LIST (ATTR_SENTINEL_NOTHROW_LEAF_LIST, ATTR_SENTINEL,	\
 DEF_ATTR_TREE_LIST (ATTR_COLD_CONST_NORETURN_NOTHROW_LEAF_LIST, ATTR_CONST,\
 			ATTR_NULL, ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST)
 
+/* Allocation functions like alloca and malloc whose first 

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

On 11/14/2016 01:34 PM, Eric Gallager wrote:

On 11/13/16, Martin Sebor  wrote:

Bug 77531 requests a new warning for calls to allocation functions
(those declared with attribute alloc_size(X, Y)) that overflow the
computation X * Z of the size of the allocated object.

Bug 78284 suggests that detecting and diagnosing other common errors
in calls to allocation functions, such as allocating more space than
SIZE_MAX / 2 bytes, would help prevent subsequent buffer overflows.

The attached patch adds two new warning options, -Walloc-zero and
-Walloc-larger-than=bytes that implement these two enhancements.
The patch is not 100% finished because, as it turns out, the GCC
allocation built-ins (malloc et al.) do not make use of the
attribute and so don't benefit from the warnings.  The tests are
also incomplete, and there's at least one bug in the implementation
I know about.

I'm posting the patch while stage 1 is still open and to give
a heads up on it and to get early feedback.  I expect completing
it will be straightforward.

Martin

PS The alloc_max_size function added in the patch handles sizes
specified using suffixes like KB, MB, etc.  I added that to make
it possible to specify sizes in excess of the maximum of INT_MAX
that (AFAIK) options that take integer arguments handle out of
the box.  It only belatedly occurred to me that the suffixes
are unnecessary if the option argument is handled using strtoull.
I can remove the suffix (as I suspect it will raise objections)
but I think that a general solution along these lines would be
useful to let users specify large byte sizes in other options
as well (such -Walloca-larger-than, -Wvla-larger-then).  Are
there any suggestions or preferences?




-Walloc-larger-than looks way too similar to -Walloca-larger-than; at
first I was confused as to why you were adding the same flag again
until I spotted the one letter difference. Maybe come up with a name
that looks more distinct? Just something to bikeshed about.


I agree.  I've renamed the option to -Walloc-size-larger-than.
I think that works because it goes along with attribute alloc_size.
I'm about to post an updated patch with that change (among others).

Thanks
Martin

Re: [PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Eric Gallager]

On 11/13/16, Martin Sebor  wrote:
> Bug 77531 requests a new warning for calls to allocation functions
> (those declared with attribute alloc_size(X, Y)) that overflow the
> computation X * Z of the size of the allocated object.
>
> Bug 78284 suggests that detecting and diagnosing other common errors
> in calls to allocation functions, such as allocating more space than
> SIZE_MAX / 2 bytes, would help prevent subsequent buffer overflows.
>
> The attached patch adds two new warning options, -Walloc-zero and
> -Walloc-larger-than=bytes that implement these two enhancements.
> The patch is not 100% finished because, as it turns out, the GCC
> allocation built-ins (malloc et al.) do not make use of the
> attribute and so don't benefit from the warnings.  The tests are
> also incomplete, and there's at least one bug in the implementation
> I know about.
>
> I'm posting the patch while stage 1 is still open and to give
> a heads up on it and to get early feedback.  I expect completing
> it will be straightforward.
>
> Martin
>
> PS The alloc_max_size function added in the patch handles sizes
> specified using suffixes like KB, MB, etc.  I added that to make
> it possible to specify sizes in excess of the maximum of INT_MAX
> that (AFAIK) options that take integer arguments handle out of
> the box.  It only belatedly occurred to me that the suffixes
> are unnecessary if the option argument is handled using strtoull.
> I can remove the suffix (as I suspect it will raise objections)
> but I think that a general solution along these lines would be
> useful to let users specify large byte sizes in other options
> as well (such -Walloca-larger-than, -Wvla-larger-then).  Are
> there any suggestions or preferences?
>


-Walloc-larger-than looks way too similar to -Walloca-larger-than; at
first I was confused as to why you were adding the same flag again
until I spotted the one letter difference. Maybe come up with a name
that looks more distinct? Just something to bikeshed about.

[PATCH] warn on overflow in calls to allocation functions (bugs 77531 and 78284)

[Martin Sebor]

Bug 77531 requests a new warning for calls to allocation functions
(those declared with attribute alloc_size(X, Y)) that overflow the
computation X * Z of the size of the allocated object.

Bug 78284 suggests that detecting and diagnosing other common errors
in calls to allocation functions, such as allocating more space than
SIZE_MAX / 2 bytes, would help prevent subsequent buffer overflows.

The attached patch adds two new warning options, -Walloc-zero and
-Walloc-larger-than=bytes that implement these two enhancements.
The patch is not 100% finished because, as it turns out, the GCC
allocation built-ins (malloc et al.) do not make use of the
attribute and so don't benefit from the warnings.  The tests are
also incomplete, and there's at least one bug in the implementation
I know about.

I'm posting the patch while stage 1 is still open and to give
a heads up on it and to get early feedback.  I expect completing
it will be straightforward.

Martin

PS The alloc_max_size function added in the patch handles sizes
specified using suffixes like KB, MB, etc.  I added that to make
it possible to specify sizes in excess of the maximum of INT_MAX
that (AFAIK) options that take integer arguments handle out of
the box.  It only belatedly occurred to me that the suffixes
are unnecessary if the option argument is handled using strtoull.
I can remove the suffix (as I suspect it will raise objections)
but I think that a general solution along these lines would be
useful to let users specify large byte sizes in other options
as well (such -Walloca-larger-than, -Wvla-larger-then).  Are
there any suggestions or preferences?
PR c/77531 - __attribute__((alloc_size(1,2))) could also warn on multiplication overflow
PR c/78284 - warn on malloc with very large arguments

gcc/c-family/ChangeLog:

	PR c/77531
	PR c/78284
	* c.opt (-Walloc-zero, -Walloc-larger-than): New options.

gcc/testsuite/ChangeLog:

	PR c/77531
	PR c/78284
	* gcc.dg/attr-alloc_size-3.c: New test.

gcc/ChangeLog:

	PR c/77531
	PR c/78284
	* calls.c (alloc_max_size): New function.
	(maybe_warn_alloc_args_overflow): Same.
	(initialize_argument_information): Diagnose overflow in functions
	declared with attaribute alloc_size.
	* doc/invoke.texi (Warning Options): Document -Walloc-zero and
	-Walloc-larger-than.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 213353b..72c1e14 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -299,6 +299,15 @@ Walloca
 C ObjC C++ ObjC++ Var(warn_alloca) Warning
 Warn on any use of alloca.
 
+Walloc-larger-than=
+C ObjC C++ ObjC++ Var(warn_alloc_limit) Warning Joined
+-Walloc-larger-than= Warn for calls to allocation functions that attempt
+to allocate objects larger than the specified number of bytes.
+
+Walloc-zero
+C ObjC C++ ObjC++ Var(warn_alloc_zero) Warning EnabledBy(Wextra)
+-Walloc-zero Warn for calls to allocation functions that specify zero bytes.
+
 Walloca-larger-than=
 C ObjC C++ ObjC++ Var(warn_alloca_limit) Warning Joined RejectNegative UInteger
 -Walloca-larger-than= Warn on unbounded uses of
diff --git a/gcc/calls.c b/gcc/calls.c
index c916e07..dfeb5fe 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -48,6 +48,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "dbgcnt.h"
 #include "rtl-iter.h"
 #include "tree-chkp.h"
+#include "tree-vrp.h"
+#include "tree-ssanames.h"
 #include "rtl-chkp.h"
 
 
@@ -1181,6 +1183,192 @@ store_unaligned_arguments_into_pseudos (struct arg_data *args, int num_actuals)
   }
 }
 
+static tree alloc_object_size_limit;
+
+/* Initialize ALLOC_OBJECT_SIZE_LIMIT based on the -Walloc-larger-than=limit
+   setting if the option is specified, or to the maximum object size if it
+   is not.  Return the initialized value.  */
+
+static tree
+alloc_max_size (void)
+{
+  if (!alloc_object_size_limit)
+{
+  alloc_object_size_limit = TYPE_MAX_VALUE (ssizetype);
+
+  unsigned HOST_WIDE_INT unit = 1;
+
+  char *end;
+  errno = 0;
+  unsigned HOST_WIDE_INT limit
+	= warn_alloc_limit ? strtoull (warn_alloc_limit, , 10) : 0;
+
+  if (limit && !errno)
+	{
+	  if (end && *end)
+	{
+	  /* Numeric option arguments are at most INT_MAX.  Make it
+		 possible to specify a larger value by accepting common
+		 suffixes.  */
+	  if (!strcmp (end, "kB"))
+		unit = 1000;
+	  else if (!strcasecmp (end, "KiB") || strcmp (end, "KB"))
+		unit = 1024;
+	  else if (!strcmp (end, "MB"))
+		unit = 1000LU * 1000;
+	  else if (!strcasecmp (end, "MiB"))
+		unit = 1024LU * 1024;
+	  else if (!strcasecmp (end, "GB"))
+		unit = 1000LU * 1000 * 1000;
+	  else if (!strcasecmp (end, "GiB"))
+		unit = 1024LU * 1024 * 1024;
+	  else if (!strcasecmp (end, "TB"))
+		unit = 1000LU * 1000 * 1000 * 1000;
+	  else if (!strcasecmp (end, "TiB"))
+		unit = 1024LU * 1024 * 1024 * 1024;
+	  else if (!strcasecmp (end, "PB"))
+		unit = 1000LU * 1000 * 1000 * 1000 * 1000;
+	  else if (!strcasecmp (end, "PiB"))
+		unit = 1024LU * 1024 *