[ubsan] nonnull and returns_nonnull sanitization

[Alexey Samsonov]

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.

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