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

[Richard Biener <rguenther at suse dot de>]

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).

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

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)