On December 8, 2018 6:42:48 PM GMT+01:00, Martin Sebor <mse...@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.
>>>
>>
