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

[Richard Biener <rguenther@suse.de>]

On December 8, 2018 6:42:48 PM GMT+01:00, Martin Sebor <msebor@gmail.com> wrote:
>On 12/7/18 1:06 AM, Richard Biener wrote:
>> On Thu, 6 Dec 2018, 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.
>> 
>> Please always _only_ use gimple_call_fntype when the decl
>> isn't visible.  As elsewhere the type of the function
>> pointer doesn't have any semantic meaning (it could be a
>> wrong one).
>
>I don't understand what you're asking me to do differently here:
>
>-  callee = gimple_call_fndecl (call);
>-  if (!callee)
>+  tree calltype;
>+  if (tree callfn = gimple_call_fndecl (call))
>+    calltype = TREE_TYPE (callfn);
>+  else
>+    calltype = gimple_call_fntype (call);
>                 ^^^^^^^^^^^^^^^^^^
>
>Can you show the change you're looking for?  (The change I had
>made originally was in response to this same request you made
>in Bugzilla, which Jakub then suggested may not be robust enough.)

I was replying to the discussion, not looking at the patch. The above snippet looks OK to me. 

>Btw., it should be straightforward to ask "give me the attribute
>if this is a call to an alloc_size-kind of a function?" (or one
>with whatever other attribute of interest).  Since it appears
>to be anything but straightforward, we should consider providing
>an API to make it so.  Maybe something like:
>
>   tree gimple_call_attribute (gcall *, tree attribute);

Maybe a char * instead of the tree argument to match lookup_attribute? But yes, sth like this looks indeed useful. 

Richard. 

>Martin
>
>> 
>> Richard.
>> 
>>> 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.
>>>
>>