Add/Update MOZ_ASAN/TSAN_BLACKLIST macros to annotate functions

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 3 bugs, {sec-want})

Trunk
mozilla27
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 844755
(Assignee)

Updated

6 years ago
Blocks: 844759
(Assignee)

Updated

6 years ago
Blocks: 844768
(Assignee)

Updated

6 years ago
Blocks: 844766
(Assignee)

Comment 1

6 years ago
Here's a link to the discussion for the sanitizer attributes on Clang.
(Assignee)

Comment 2

6 years ago
Created attachment 721759 [details] [diff] [review]
Patch

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
Status: NEW → ASSIGNED
Attachment #721759 - Flags: review?(ted)
Comment on attachment 721759 [details] [diff] [review]
Patch

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 4

6 years ago
Comment on attachment 721759 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 5

5 years ago
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 configure.in 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).
Blocks: 863846
Summary: Add MOZ_TSAN_BLACKLIST macro to annotate functions → Add/Update MOZ_ASAN/TSAN_BLACKLIST macros to annotate functions
(Assignee)

Comment 6

5 years ago
Created attachment 818306 [details] [diff] [review]
asan-tsan-attr.patch

Waldo, can you re-review this simple patch, since it changed quite a bit since the last patch? With all the configure.in bits and the blacklist gone, your review should be sufficient :) Thanks!
Attachment #721759 - Attachment is obsolete: true
Attachment #818306 - Flags: review?(jwalden+bmo)

Comment 7

5 years ago
Comment on attachment 818306 [details] [diff] [review]
asan-tsan-attr.patch

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

::: mfbt/Attributes.h
@@ +175,5 @@
> +# if __has_feature(address_sanitizer)
> +#  undef MOZ_ASAN_BLACKLIST
> +#  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
#else
#  define MOZ_ASAN_BLACKLIST /* nothing */
#endif

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+
(Assignee)

Comment 8

5 years ago
Created attachment 818465 [details] [diff] [review]
asan-tsan-attr.patch

Updated patch according to reviewer comments. Carrying r+.
Attachment #818306 - Attachment is obsolete: true
Attachment #818465 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/315b927c1934
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Updated

5 years ago
Blocks: 929478
You need to log in before you can comment on or make changes to this bug.