Closed Bug 847350 Opened 10 years ago Closed 9 years ago

Add/Update MOZ_ASAN/TSAN_BLACKLIST macros to annotate functions


(Core :: General, defect)

Not set





(Reporter: decoder, Assigned: decoder)


(Blocks 2 open bugs, )


(Keywords: sec-want)


(1 file, 2 obsolete files)

In order to make TSan ignore functions during instrumentation (compile time), we currently have to use the sanitize blacklist (-fsanitize-blacklist="blacklist file"). However, we need a macro to annotate functions anyway for two reasons

1) Functions that are marked this way must not be inlined (MOZ_NEVER_INLINE)
2) Soon, blacklisting will be done using an attribute (like ASan currently uses __attribute__((no_address_safety_analysis)) right now) instead of a file.

Patch coming soon.
Blocks: 844755
Blocks: 844759
Blocks: 844768
Blocks: 844766
Here's a link to the discussion for the sanitizer attributes on Clang.
Attached patch Patch (obsolete) — Splinter Review
This patch adds the macro itself, a necessary configure switch to control it and also a preliminary blacklist file (new file) that will be used to store all the blacklist entries until this can be replaced with the compiler attribute. Until that, the annotation has to be used together with -fsanitize-blacklist to work.
Assignee: nobody → choller
Attachment #721759 - Flags: review?(ted)
Comment on attachment 721759 [details] [diff] [review]

Review of attachment 721759 [details] [diff] [review]:

You should get a MFBT peer to sign off on the MFBT change. The build stuff looks fine.
Attachment #721759 - Flags: review?(ted)
Attachment #721759 - Flags: review?(jwalden+bmo)
Attachment #721759 - Flags: review+
Comment on attachment 721759 [details] [diff] [review]

Review of attachment 721759 [details] [diff] [review]:

Seems reasonable.

::: mfbt/Attributes.h
@@ +164,5 @@
> + * instrumentation shipped with Clang) to not instrument the annotated function.
> + * Right now, it only prevents the compiler from inlining the function (since
> + * inlining breaks the blacklisting mechanism) and blacklisting has to be done
> + * with an external file, but in the future, an attribute like AddressSanitizer
> + * has one will be used here.

For the last bit, do you mean "an attribute like __attribute__((no_thread_safety_analysis)) will be used as soon as Clang supports one" or something like that?  This doesn't parse right now.  :-)
Attachment #721759 - Flags: review?(jwalden+bmo) → review+
The attribute has changed since this patch was made (both for ASan and TSan, and the old ones are deprecated). I'll come up with a new patch that updates the flags. The patch will also use a new approach using __has_feature instead of requiring MOZ_TSAN through at all. Furthermore, the same can be done for MOZ_ASAN_BLACKLIST, so the ASan annotations will not be used if the compiler doesn't understand them (this is right now the case when building on Windows for ASan where ASan is a post-build step, --enable-address-sanitizer causes breakage there).

I will also not land a blacklist with this patch, because for the compile-time blacklisting, the annotations are sufficient now (at least on Clang trunk).
Summary: Add MOZ_TSAN_BLACKLIST macro to annotate functions → Add/Update MOZ_ASAN/TSAN_BLACKLIST macros to annotate functions
Attached patch asan-tsan-attr.patch (obsolete) — Splinter Review
Waldo, can you re-review this simple patch, since it changed quite a bit since the last patch? With all the bits and the blacklist gone, your review should be sufficient :) Thanks!
Attachment #721759 - Attachment is obsolete: true
Attachment #818306 - Flags: review?(jwalden+bmo)
Comment on attachment 818306 [details] [diff] [review]

Review of attachment 818306 [details] [diff] [review]:

::: mfbt/Attributes.h
@@ +175,5 @@
> +# if __has_feature(address_sanitizer)
> +#  define MOZ_ASAN_BLACKLIST MOZ_NEVER_INLINE __attribute__((no_sanitize_address))
> +# endif
> +#endif

I'd rather see

#if defined(__has_feature)
#  if __has_feature(address_sanitizer)
#    define MOZ_ASAN_BLACKLIST MOZ_NEVER_INLINE __attribute__((no_sanitize_address))
#  else
#    define MOZ_ASAN_BLACKLIST /* nothing */
#  endif
#  define MOZ_ASAN_BLACKLIST /* nothing */

so that people can read the if/else/etc. code to see how the value is defined, rather than having a default and then overriding it after.  (Also: note two-space indentation of nested preprocessor directives.

@@ +193,1 @@
>  #endif

Same rejiggering here, and two-space indents.
Attachment #818306 - Flags: review?(jwalden+bmo) → review+
Updated patch according to reviewer comments. Carrying r+.
Attachment #818306 - Attachment is obsolete: true
Attachment #818465 - Flags: review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: tsan
You need to log in before you can comment on or make changes to this bug.