[PATCH] Check for null pointers given to memcpy with ubsan

[Nuno Lopes]

> On 24 May 2015 4:57 pm, "Nuno Lopes" <nunoplopes at sapo.pt> wrote:
>>
>> In http://reviews.llvm.org/D9673#170543, @samsonov wrote:
>>
>> > In http://reviews.llvm.org/D9673#170474, @nlopes wrote:
>> >
>> > > > Wait a second, won't UBSan handle this automatically if 
>> > > > memcpy/memmove are
>> > >
>> > > >  declared with __attribute__((nonnull)) in the header? Otherwise, 
>> > > > is there
>> > >
>> > > >  a change to the standard that imposes these additional constraints 
>> > > > on
>> > >
>> > > >  memcpy/memmove?
>> > >
>> > >
>> > > Not really. memcpy/memmove calls are handled by CGBultin and not 
>> > > CGCall.
>> > >  It's a different code path.
>> > > Nuno
>> >
>> >
>> > Interesting. I think that if we decide to implement such a check, we 
>> > shouldn't depend on
>> >  attributes specified in the headers, so `nonnull-attribute` is no 
>> > longer relevant. There are another
>> >  kind of compiler builtins which worth extra checks, and which don't 
>> > even require headers - e.g. behavior of __builtin_ctz(0)
>> >  is undefined. I think we should implement another check kind 
>> > `-fsanitize=builtin` that would verify arguments of
>> >  various builtin functions.
>>
>>
>> Well, I think in the end they are all in the UB category, so I think they 
>> fall in the scope of -fsanitize=undefined.
>
> We have sanitizer groups (much like warning groups); nothing goes directly 
> in -fsanitize=undefined.

Sure. This patch reuses the same code path as checking for null arguments in 
functions.  Not sure if it justifies a separate flag in -fsanitize for 
builtins instead of splitting these checks amongst the current set of 
categories.
BTW I've already used this patch to catch multiple crashes in an application 
due to gcc 4.9 optimizing memcpy with null args.  Hence I think we should 
have those checks in place to help people (myself included :).

Are you ok with the patch?

Nuno