[ubsan] nonnull and returns_nonnull sanitization

[Alexey Samsonov]

Ok, looks like I wasn't updating this thread for a while, sorry about that.
The current status is:
-fsanitize=returns-nonnull-attribute is implemented in Clang trunk
-fsanitize=nonnull-attribute is now being reviewed (
http://reviews.llvm.org/D5082). There were delays here due to numerous
problems with __attribute__((nonnull)) implementation in Clang and
uncertainties about nonnull semantic:

  __attribute__((nonnull)) void foo(int a, ...);
  void call_foo() { foo(42, (void*)0); }  //should we report error in this
case? Probably yes, as GCC prints compile-time warning here.

Now, the question is about the __ubsan_handle_nonnull_arg parameters.
Originally you proposed, to pass SourceLocation of a call in static data
and argument index in dynamic data (to be able to merge locations).
However, I think the error reports can benefit if we:
1) pinpoint the location of a specific argument that incorrectly evaluates
to NULL.
2) print the location of a function declaration which contains the nonnull
attribute.
Note that if we do (1) we might as well pass argument index in static data,
as locations of different arguments will not be merged together. Richard
also raised a valid concern that passing argument index increases the code
size instead of the .data, which might be not desirable for functions with
few nonnull arguments.





On Mon, Aug 11, 2014 at 1:29 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:

>
> On Mon, Aug 11, 2014 at 12:39 PM, Jakub Jelinek <jakub at redhat.com> wrote:
>
>> On Mon, Aug 11, 2014 at 12:05:00PM -0700, Alexey Samsonov wrote:
>> > 1) Frontend flag naming. We can add new values:
>> -fsanitize=returns-nonnull
>> > and -fsanitize=nonnull. But it looks confusing to
>> > me: having both "-fsanitize=null" and "-fsanitize=nonnull" is weird.
>> Clang
>> > user manual tells that -fsanitize=null detects
>> > "Use of a null pointer or creation of a null reference." Maybe, we
>> should
>> > just put both new checks under "-fsanitize=null"
>> > and make manual say "invalid use of a null pointer or creation of a null
>> > reference". Passing null pointer to function annotated
>> > with __attribute__((nonnull)) conforms to "invalid use of a null
>> pointer".
>> > As an alternative, we can introduce new "-fsanitize=nonnull-attributes"
>> for
>> > both new checks.
>>
>> Sticking it under null means limiting user's choice, and just passing or
>> returning a pointer isn't a use of the pointer.  As one attribute is
>> nonnull
>> and another is returns_nonnull, calling them nonnull-attributes is weird
>> to
>> me.  I don't find nonnull/returns-nonnull confusing, but could live with
>> nonnull-attr/returns-nonnull-attr or
>> nonnull-attribute/returns-nonnull-attribute.
>> Perhaps nonnull could enable also those two, but then it would be a group
>> name as well as sanitization name, which would confuse users too.
>>
>
> Personally, I also feel that fine granularity is important for UBSan
> options
> (and is a huge help in tool deployment), but let's agree with Richard on
> that.
>
>
>>
>> > 2) Handler arguments. Is there any particular reason to not include
>> ArgNo
>> > into NonNullArgData? Is it done to reduce the number of function calls /
>> > basic blocks?
>>
>> Yeah.  Many functions have several nonnull arguments, including memcpy,
>> memmove etc., and nonnull attribute without arguments after all means all
>> pointer arguments must be nonnull.  So, by not sticking the ArgNo into
>> the structure the same data can be used for several calls.
>>
>
> Sure, makes sense.
>
>
>>
>> > Another question is the meaning of SourceLocation in NonNullArgData and
>> > NonNullRetData. I assume that SourceLocation in the first one should
>> point
>> > to the
>> > invalid call of function with attributes, while SourceLocation in the
>> > second one should point to the bad return statement inside the function
>> > with attributes. Is that so?
>>
>> Sure, it is the locus of the call statement for nonnull attribute and
>> locus of the return statement on returns_nonnull attribute.
>>
>>         Jakub
>>
>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com
>



-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140827/eb05b168/attachment.html>