[ubsan] nonnull and returns_nonnull sanitization

[Alexey Samsonov]

On Wed, Sep 10, 2014 at 11:50 AM, Alexey Samsonov <vonosmas at gmail.com>
wrote:

>
> On Tue, Sep 9, 2014 at 4:39 AM, Jakub Jelinek <jakub at redhat.com> wrote:
>
>> On Mon, Sep 08, 2014 at 01:32:02PM -0700, Alexey Samsonov wrote:
>> > OK. I've submitted the patches to Clang and UBSan runtime which
>> implement
>> > -fsanitize=nonnull-attribute and -fsanitize=returns-nonnull-attribute
>> (the
>> > last
>> > commit is r217400). I've added source locations of attribute
>> declarations to
>> > the static data passed in UBSan handlers to print them in the error
>> > reports: it makes
>> > sense to actually show user the declaration which forbids
>> passing/returning
>> > null pointer.
>> > This location is also added to returns-nonull attribute, because the
>> > attribute might be
>> > declared far from the actual function definition with incorrect
>> > return-statement.
>>
>> Thanks.  BTW, the recent changes to move Die() call out of the
>> __ubsan_*_abort handlers look wrong to me, the functions are no longer
>> noreturn if for whatever reason isDisabled would return true on the
>> location, but the compiler rightfully assumes those functions never
>> return.
>> If you want to avoid dying without emitting messages, you could e.g.
>> ignore
>> isDisabled if abort is set in the opts.
>>
>
> Ah, it's a great point. This one is tricky. Currently, "Loc.isDisabled()"
> is used only
> for report deduplication so that we don't report the same error twice.
>
> If we're building the code with fatal UBSan handlers (with
> -fno-sanitize-recover), then
> the first error we report won't have disabled source location and we will
> crash the
> program immediately after reporting it. There's still a possibility for
> undefined behavior, though:
> 1) Thread 1 enters __ubsan_handle_foo(), acquires the source location and
> starts to format error report
> 2) Thread 2 enters the same __ubsan_handle_foo(), sees that the source
> location is disabled, and exits
>     from a noreturn function, causing an undefined behavior.
>
> So, looks like we should indeed skip "Loc.isDisabled()" check in _abort
> mode - in the case described above
> we will just spin on the report mutex in Thread 2.
>
> That said, I dislike all these _abort functions - it would be cleaner to
> control UBSan behavior with a runtime flag,
> but probably we have to live with it, as I guess NORETURN helps a lot in
> terms of code size and runtime overhead.
>

r217542 adds "noreturn" attribute to fatal handlers declarations and adds
back "Die()" calls to the end of them.


>
> --
> 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/20140910/01518ca0/attachment.html>