Re: [PATCH] handle function pointers in __builtin_object_size (PR 88372)

[Jeff Law <law at redhat dot com>]

On 12/6/18 4:01 PM, Martin Sebor wrote:
> On 12/6/18 2:26 PM, Jakub Jelinek wrote:
>> On Thu, Dec 06, 2018 at 01:21:58PM -0700, Martin Sebor wrote:
>>> Bug 88372 - alloc_size attribute is ignored on function pointers
>>> points out that even though the alloc_size attribute is accepted
>>> on function pointers it doesn't have any effect on Object Size
>>> Checking.  The reporter, who is implementing the feature in Clang,
>>> wants to know if by exposing it under the same name they won't be
>>> causing incompatibilities with GCC.
>>>
>>> I don't think it's intentional that GCC doesn't take advantage of
>>> the attribute for Object Size Checking, and certainly not to detect
>>> the same kinds of issues as with other allocation functions (such
>>> as excessive or negative size arguments).  Rather, it's almost
>>> certainly an oversight since GCC does make use of function pointer
>>> attributes in other contexts (e.g., attributes alloc_align and
>>> noreturn).
>>>
>>> As an oversight, I think it's fair to consider it a bug rather
>>> than a request for an enhancement.  Since not handling
>>> the attribute in Object Size Checking has adverse security
>>> implications, I also think this bug should be addressed in GCC
>>> 9.  With that, I submit the attached patch to resolve both
>>> aspects of the problem.
>>
>> This is because alloc_object_size has been written before we had attributes
>>
>> like alloc_size.  The only thing I'm unsure about is whether we should
>> prefer gimple_call_fntype or TREE_TYPE (gimple_call_fndecl ()) if it is a
>> direct call or if we should try to look for alloc_size attribute on both
>> of those if they are different types.  E.g. if somebody does
>>
>> #include <stdlib.h>
>>
>> typedef void *(*allocfn) (size_t);
>>
>> static inline void *
>> foo (allocfn fn, size_t sz)
>> {
>>    return fn (sz);
>> }
>>
>> static inline void *
>> bar (size_t sz)
>> {
>>    return foo (malloc, sz);
>> }
>>
>> then I think this patch would no longer treat it as malloc.
>>
>> As this is security relevant, I'd probably look for alloc_size
>> attribute in both gimple_call_fntype and, if gimple_call_fndecl is non-NULL,
>>
>> its TREE_TYPE.
> 
> Thanks for the test case!  I wondered if using fntype would
> always work but couldn't think of when it wouldn't.  I've
> adjusted the function to use both and added the test case.
> 
> While thinking about this it occurred to me that alloc_size
> is only documented as a function attribute but not one that
> applies to pointers or types.  I added documentation for
> these uses to the Common Type and Common Variable sections.
> 
> Martin
> 
> PS Other function attributes that also apply to types and
> variables are only documented in the function section.  They
> should also be mentioned in the other sections.  Which, if
> done in the established style, will result in duplicating
> a lot of text in three places.  I think that suggests that
> we might want to think about structuring these sections of
> the manual differently to avoid the duplication.
> 
> gcc-88372.diff
> 
> PR tree-optimization/88372 - alloc_size attribute is ignored on function pointers
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/88372
> 	* calls.c (maybe_warn_alloc_args_overflow): Handle function pointers.
> 	* tree-object-size.c (alloc_object_size): Same.  Simplify.
> 	* doc/extend.texi (Object Size Checking): Update.
> 	(Other Builtins): Add __builtin_object_size.
> 	(Common Type Attributes): Add alloc_size.
> 	(Common Variable Attributes): Ditto.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/88372
> 	* gcc.dg/Walloc-size-larger-than-18.c: New test.
> 	* gcc.dg/builtin-object-size-19.c: Same.
OK
jeff