Integrate AddressSanitizer changes into mozilla-central

RESOLVED FIXED in mozilla13

Status

()

Core
Security
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

Other Branch
mozilla13
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
We previously managed to get Firefox to run with ASan (Address Sanitizer, see also bug 664901), however, right now there are still at least two patches required. I'd like to discuss in this bug how we should integrate the changes in mozilla-central.

The first change is a change to the security/manager/Makefile.in, roughly as follows:

 DEFAULT_GMAKE_FLAGS += FREEBL_NO_DEPEND=0
+DEFAULT_GMAKE_FLAGS += ARCHFLAG="$(CFLAGS)";
 ifeq ($(OS_TARGET),Linux)
 DEFAULT_GMAKE_FLAGS += FREEBL_LOWHASH=1
 endif

It ensures that the CFLAGS specified in the mozconfig are also passed to NSS, which is mandatory. Is this the right way to do it and should we just guard this addition with a variable set by some mozconfig option (e.g. --enable-address-sanitizer ?) which could also set the flags appropriately?.

The second change is a JS related change, it changes the maximum stack depth in two files:

In js/xpconnect/src/XPCJSRuntime.cpp:

-        JS_SetNativeStackQuota(mJSRuntime, 128 * sizeof(size_t) * 1024);
+        JS_SetNativeStackQuota(mJSRuntime, 4 * 128 * sizeof(size_t) * 1024);

And in dom/workers/RuntimeService.cpp:

-#define WORKER_CONTEXT_NATIVE_STACK_LIMIT 128 * sizeof(size_t) * 1024
+#define WORKER_CONTEXT_NATIVE_STACK_LIMIT 2 * 128 * sizeof(size_t) * 1024

Again, of course this should be guarded by a build variable. However, I'd also like some JS people to quickly check if this is sound, as the code and stack size calculation changed a few times.


A third thing is related to build flags again: Currently we pass -Wl,-z,defs to all shared libraries we link. There is no way to disable that, but it's incompatible with ASan as ASan depends on having unresolved symbols in .so files. I solved this for now with a Clang patch and I hope that the Clang developers will push a similar solution upstream, but in case we can solve the issue here, that would also be suitable of course.
(Assignee)

Updated

6 years ago
Summary: Run AddressSanitizer on Firefox → Integrate AddressSanitizer changes into mozilla-central
(In reply to Christian Holler (:decoder) from comment #0)
> We previously managed to get Firefox to run with ASan (Address Sanitizer,
> see also bug 664901), however, right now there are still at least two
> patches required. I'd like to discuss in this bug how we should integrate
> the changes in mozilla-central.
> 
> The first change is a change to the security/manager/Makefile.in, roughly as
> follows:
> 
>  DEFAULT_GMAKE_FLAGS += FREEBL_NO_DEPEND=0
> +DEFAULT_GMAKE_FLAGS += ARCHFLAG="$(CFLAGS)";
>  ifeq ($(OS_TARGET),Linux)
>  DEFAULT_GMAKE_FLAGS += FREEBL_LOWHASH=1
>  endif
> 
> It ensures that the CFLAGS specified in the mozconfig are also passed to
> NSS, which is mandatory. Is this the right way to do it and should we just
> guard this addition with a variable set by some mozconfig option (e.g.
> --enable-address-sanitizer ?) which could also set the flags appropriately?.

Not passing our CFLAGS into NSS is intentional, and I'd be very wary of changing that.

> The second change is a JS related change, it changes the maximum stack depth
> in two files:
> 
> In js/xpconnect/src/XPCJSRuntime.cpp:
> 
> -        JS_SetNativeStackQuota(mJSRuntime, 128 * sizeof(size_t) * 1024);
> +        JS_SetNativeStackQuota(mJSRuntime, 4 * 128 * sizeof(size_t) * 1024);
> 
> And in dom/workers/RuntimeService.cpp:
> 
> -#define WORKER_CONTEXT_NATIVE_STACK_LIMIT 128 * sizeof(size_t) * 1024
> +#define WORKER_CONTEXT_NATIVE_STACK_LIMIT 2 * 128 * sizeof(size_t) * 1024
> 
> Again, of course this should be guarded by a build variable. However, I'd
> also like some JS people to quickly check if this is sound, as the code and
> stack size calculation changed a few times.

Some reasoning for your constants would be good here ...

> A third thing is related to build flags again: Currently we pass -Wl,-z,defs
> to all shared libraries we link. There is no way to disable that, but it's
> incompatible with ASan as ASan depends on having unresolved symbols in .so
> files. I solved this for now with a Clang patch and I hope that the Clang
> developers will push a similar solution upstream, but in case we can solve
> the issue here, that would also be suitable of course.

I don't think we want to drop -z,defs from our shared libraries.
(Assignee)

Comment 2

6 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)

First of all, as discussed on IRC, all of these changes would be off by default, and only enabled with a special build flag.

> Not passing our CFLAGS into NSS is intentional, and I'd be very wary of
> changing that.

We discussed this on IRC and it seems a better solution to hardcode the ASan flags directly in the ARCHFLAG variable, rather than passing all CFLAGS. Of course we'd have to check how to handle the blacklist there which is also part of CFLAGS. I'll try to come up with a patch.

 
> Some reasoning for your constants would be good here ...

Initially, starting Firefox failed with too much recursion errors. I had to at least increase the stack size by a factor of 4 (on startup, the parser seems to generate very large stack frames with ASan enabled). The second change is just a guess because the stack will be larger (every variable gets a redzone). I'll try to find out again if *4 for the native size is required or if a lower value works by now (lots of code has been refactored since then).

 
> I don't think we want to drop -z,defs from our shared libraries.

As discussed on IRC, we'll try to do this when building with ASan. However, if it's very complicated and has to be done in a lot of places, then I'd vote for a solution on the Clang/ASan side instead.
(Assignee)

Comment 3

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

It took me quite a while, but me and the build system are getting closer friends it seems^^ The patch does the following:

1. It adds a flag --enable-address-sanitizer which activates code workarounds (this is right now only the JS stack space increasement in two places).

2. It adds a flag --enable-cflags-nss that forces passing CFLAGS to NSS. I quickly realized that the solution discussed before (hardcoding ASan flags in security/manager/Makefile.in) isn't good for two reasons: First we would have the flags in two places (we still need them in .mozconfig additionally), and secondly, every other LLVM pass that requires compiler flags (e.g. my LLCov tool), requires exactly the same functionality (i.e. passing the flags also to NSS).

3. It adds --enable-undefined-so-symbols which prevents the use of "-Wl,-z,defs" during building of shared libraries in various places. Again, this is not ASan specific because every LLVM pass using a runtime library like ASan does, will require this change.


With these changes, it is possible to build with Address Sanitizer by using these three options and including "-faddress-sanitizer -Dxmalloc=myxmalloc -mllvm -asan-blacklist=<blacklist file>" in CFLAGS/CXXFLAGS, and "-faddress-sanitizer" in LDFLAGS. Of course you still need to disable jemalloc and the crash reporter as usual :)

Looking forward to feedback.
Assignee: nobody → choller
Status: NEW → ASSIGNED
Attachment #598038 - Flags: review?(khuey)
Attachment #598038 - Flags: feedback?(bhackett1024)
Will this require and/or check for a fixed or minimum version of LLVM?
(Assignee)

Comment 5

6 years ago
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #4)
> Will this require and/or check for a fixed or minimum version of LLVM?

As for checking: It will not check right now because you set the CC and CFLAGS yourself. The build flags don't enable any compiler options, they just enable the necessary workarounds.

As for requirements: It will require Clang >=3.0 with ASan included (it got included at some point on trunk) + the compiler-rt. But it will not require any additional patches anymore after this patch has landed.
Comment on attachment 598038 [details] [diff] [review]
Patch

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

::: configure.in
@@ +2218,5 @@
> +    MOZ_ASAN= )
> +if test -n "$MOZ_ASAN"; then
> +    AC_DEFINE(MOZ_ASAN)
> +fi
> +AC_SUBST(MOZ_ASAN)

I would rather we have a single configure option, --enable--address-sanitizer, that does this other stuff, since there's no use for --enable-cflags-nss or --enable-undefined-so-symbols without it.

r- for this.

::: dom/workers/RuntimeService.cpp
@@ +94,2 @@
>  #define WORKER_CONTEXT_NATIVE_STACK_LIMIT 128 * sizeof(size_t) * 1024
> +#endif

Someone from the JS team should sign off on this.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1970,2 @@
>          JS_SetNativeStackQuota(mJSRuntime, 128 * sizeof(size_t) * 1024);
> +#endif

Ditto.

::: security/coreconf/Linux.mk
@@ +159,1 @@
>  LDFLAGS			+= $(ARCHFLAG)

This is in NSS, so it will need to be reviewed by bsmith or someone from the NSS team and handled as a separate patch.
Attachment #598038 - Flags: review?(khuey) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 600016 [details] [diff] [review]
Updated patch

Updated patch per review comments:

- As discussed on IRC, the nss cflags + no-wlzdefs parts should remain separate from --enable-address-sanitizer but are now grouped as --enable-llvm-hacks, which is automatically enabled with address sanitizer.

- Splitted out the NSS part (security/coreconf/Linux.mk) for separate review/checkin on NSS.

Also including Luke for feedback on the JS engine stack size doubling when ASan is active.
Attachment #598038 - Attachment is obsolete: true
Attachment #600016 - Flags: review?(khuey)
Attachment #600016 - Flags: feedback?(luke)
Attachment #598038 - Flags: feedback?(bhackett1024)
Comment on attachment 600016 [details] [diff] [review]
Updated patch

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

r=me on the build stuff
Attachment #600016 - Flags: review?(khuey) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 600019 [details] [diff] [review]
NSS part of the patch

This is the NSS part of the patch, which just allows disabling -Wl,-z,defs by defining MOZ_NO_WLZDEFS.
Attachment #600019 - Flags: review?(kaie)

Comment 10

6 years ago
Comment on attachment 600019 [details] [diff] [review]
NSS part of the patch

>diff --git a/security/coreconf/Linux.mk b/security/coreconf/Linux.mk

>+ifndef MOZ_NO_WLZDEFS
> DSO_LDOPTS		+= $(if $(findstring 2.11.90.0.8,$(shell ld -v)),,$(ZDEFS_FLAG))
>+endif

I haven't reviewed to this file before.
But given you're introducing a new variable, which is not being anywhere else in NSPR/NSS, and given you're not changing the default, this looks ok to me.

I'm giving you a tentative r=kaie

but Wan-Teh often has good thoughts around such changes, 
so you might consider to wait for his feedback.
Attachment #600019 - Flags: review?(kaie)
Attachment #600019 - Flags: review+
Attachment #600019 - Flags: feedback?(wtc)

Updated

6 years ago
Attachment #600016 - Flags: feedback?(luke) → feedback+

Comment 11

6 years ago
Comment on attachment 600019 [details] [diff] [review]
NSS part of the patch

> ZDEFS_FLAG		= -Wl,-z,defs

You can accomplish the goal by doing
    ZDEFS_FLAG=
in mozilla/security/manager/Makefile.in.
You are already overriding the NSS internal build variable
ARCHFLAG this way to pass your $(CFLAGS) to NSS.

>+ifndef MOZ_NO_WLZDEFS
> DSO_LDOPTS		+= $(if $(findstring 2.11.90.0.8,$(shell ld -v)),,$(ZDEFS_FLAG))
>+endif

I think LD_ALLOW_UNDEFINED or LINKER_ALLOW_UNDEFINED would
be easier to understand than MOZ_NO_WLZDEFS.  It's not easy
to guess WLZDEFS refers to -Wl,-z,defs.
(Assignee)

Comment 12

6 years ago
(In reply to Wan-Teh Chang from comment #11)
> 
> You can accomplish the goal by doing
>     ZDEFS_FLAG=
> in mozilla/security/manager/Makefile.in.

Hm, I'm pretty sure I tried something like this, but I'm going to give it another try, thanks! :) That would make things easier.

> I think LD_ALLOW_UNDEFINED or LINKER_ALLOW_UNDEFINED would
> be easier to understand than MOZ_NO_WLZDEFS.  It's not easy
> to guess WLZDEFS refers to -Wl,-z,defs.

Alright, I'll think about renaming it :)

If your hint works, then I'm not going to have to patch NSS at all, which would be great :)
(Assignee)

Comment 13

6 years ago
Created attachment 603671 [details] [diff] [review]
Updated patch

This updated patch uses the ZDEFS_FLAG approach that wtc suggested, which makes the NSS change unnecessary. Also, it uses XCFLAGS instead of ARCHFLAG to pass CFLAGS to NSS per suggestion of wtc via email. XCFLAGS is less intrusive as it does not simply overwrite what NSS coreconf assigns to ARCHFLAG.
Attachment #600016 - Attachment is obsolete: true
Attachment #600019 - Attachment is obsolete: true
Attachment #600019 - Flags: feedback?(wtc)
(Assignee)

Comment 14

6 years ago
Comment on attachment 603671 [details] [diff] [review]
Updated patch

Keeping r+ from first patch, as discussed on IRC.
Attachment #603671 - Flags: review+
Attachment #603671 - Flags: checkin?(gary)
Comment on attachment 603671 [details] [diff] [review]
Updated patch

http://hg.mozilla.org/integration/mozilla-inbound/rev/ad79ffdf94a3
Attachment #603671 - Flags: checkin?(gary) → checkin+
https://hg.mozilla.org/mozilla-central/rev/ad79ffdf94a3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.