[PATCH] Add a no_sanitize_vptr function attribute.

[Aaron Ballman]

On Wed, Apr 22, 2015 at 4:12 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>
>
> On Wed, Apr 22, 2015 at 12:37 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Mon, Apr 20, 2015 at 9:06 PM, Oliver Chang <ochang at google.com> wrote:
>> > In http://reviews.llvm.org/D9059#158733, @samsonov wrote:
>> >
>> >> In http://reviews.llvm.org/D9059#157697, @ochang wrote:
>> >>
>> >> > In http://reviews.llvm.org/D9059#157331, @rsmith wrote:
>> >> >
>> >> > > I'm not convinced that adding one attribute per sanitizer is the
>> >> > > right design here -- it doesn't seem to scale very well. Have you considered
>> >> > > an attribute like
>> >> > >
>> >> > >   __attribute__((no_sanitize("list,of,sanitizers"))) void fn() {
>> >> > > ... }
>> >> > >
>> >> > >
>> >> > > where the list is parsed as if it were specified as
>> >> > > `-fno-sanitize=list,of,sanitizers` on the command line?
>> >> >
>> >> >
>> >> > This does seem like a much better way of doing this. Should I change
>> >> > this in this patch?
>> >> >
>> >> > What does everyone else think?
>> >>
>> >>
>> >> I agree with this suggestion. It would be cool to have a single
>> >> attribute like this, and later deprecate no_sanitize_address,
>> >> no_sanitize_thread and no_sanitize_memory.
>> >
>> >
>> > I can put together another patch in the next few days, unless someone
>> > else (more experienced) wants to take this?
>>
>> I'd like to make sure I'm on the same page for the design. I was under
>> the impression that your original patch was attempting to suppress a
>> feature of a sanitizer, not an entire sanitizer itself. I think the
>> suggestions have been that we should have a single attribute which
>> allows you to turn off individual ubsan sanitizer features, and
>> another attribute (someday) to turn of entire sanitizers (which
>> replaces no_sanitize_thread, etc).
>
>
> Not really. There are two kinds of arguments to -fsanitize= options:
> 1) inidividual sanitizers ("address", "thread", "vptr", etc.)
> 2) sets of sanitizers ("undefined", "cfi", etc.).
>
> We can introduce a single no_sanitize("list", "of", "sanitizers") that would
> be flexible
> enough for your use cases:
> [[clang::no_sanitize("vptr", "shift")]]  // disable just "vptr" and "shift"
> checks in the function
> [[clang::no_sanitize("undefined")]]  // disable all checks introduced by
> UBSan (-fsanitize=undefined)
> [[clang::no_sanitize("undefined", "address", "memory")]]  // Disable UBSan,
> MSan, ASan instrumentation (but use TSan, if the code will be compiled with
> -fsanitize=thread).
>
> Then no_sanitize_address will be just a synonym for no_sanitize("address"),
> etc.

To me, using the existing attributes makes more sense. For starters,
they're self-documenting as to what sanitizer is being affected. This
means we don't have to be as careful about adding selective features
to the sanitizers (the namespace becomes the sanitizer, and not all
sanitizers).

Also, how much do we have to worry about feature names across
sanitizers? e.g., asan and msan both share a feature that does
slightly different things under the same name?

For background, eventually I want to move the sanitizer attributes out
of clang entirely (by having a plugin system for attributes), and have
them under the control of the individual sanitizers. Having all
sanitizers listed together makes this goal considerably more
challenging. By having each sanitizer segregate its own list of
features, it makes a more clear layering.

~Aaron

>
>> I'm wondering if we can do
>> something like this:
>>
>> 1) Add a new no_sanitize_undefined attribute that turns off all ubsan
>> checks for a method, and,
>> 2) Have the attribute take an optional, variadic list of sanitizer
>> feature strings to fine-tune what gets turned off.
>> 3) Someday, if needed, add the optional list functionality to the
>> no_sanitize_address, no_sanitize_memory, and no_sanitize_thread
>> attributes.
>> 4) Someday add [[clang::no_sanitize("list", "of", "sanitizers")]] for
>> turning off entire sanitizers.
>>
>> This allows you to do:
>>
>> [[clang::no_sanitize_undefined("vptr", "something_else")]] void f() {}
>> // Turns off just vptr and something_else ubsan instrumentation
>> [[clang::no_sanitize_undefined]] void g() {} // Turns off all ubsan
>> instrumentation for this function
>>
>> // Someday
>> [[clang::no_sanitize("ubsan", "asan")]] void h() {} // Turns off all
>> ubsan and asan instrumentation, but allows tsan and msan.
>>
>> I think this provides maximal flexibility with minimal worry about
>> name clashes between sanitizer features. It comes with the handy
>> benefit of not needing to deprecate the no_sanitize_blah attributes,
>> but still scales.
>>
>> ~Aaron
>>
>> >
>> >
>> > http://reviews.llvm.org/D9059
>> >
>> > EMAIL PREFERENCES
>> >   http://reviews.llvm.org/settings/panel/emailpreferences/
>> >
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com