[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 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?
>>
>>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-78284.diff
Type: text/x-patch
Size: 68953 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20161120/95ca608b/attachment.bin>