Last Comment Bug 727445 - Integrate AddressSanitizer changes into mozilla-central
: Integrate AddressSanitizer changes into mozilla-central
Status: RESOLVED FIXED
[sg:want]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Other Branch
: x86 Linux
: -- normal (vote)
: mozilla13
Assigned To: Christian Holler (:decoder)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on:
Blocks: 664901
  Show dependency treegraph
 
Reported: 2012-02-15 07:29 PST by Christian Holler (:decoder)
Modified: 2012-03-08 13:55 PST (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.39 KB, patch)
2012-02-16 15:48 PST, Christian Holler (:decoder)
khuey: review-
Details | Diff | Splinter Review
Updated patch (8.83 KB, patch)
2012-02-23 07:36 PST, Christian Holler (:decoder)
khuey: review+
luke: feedback+
Details | Diff | Splinter Review
NSS part of the patch (1.02 KB, patch)
2012-02-23 07:39 PST, Christian Holler (:decoder)
kaie: review+
Details | Diff | Splinter Review
Updated patch (8.84 KB, patch)
2012-03-07 04:14 PST, Christian Holler (:decoder)
choller: review+
gary: checkin+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-02-15 07:29:27 PST
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.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-15 07:39:28 PST
(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.
Comment 2 Christian Holler (:decoder) 2012-02-15 08:09:30 PST
(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.
Comment 3 Christian Holler (:decoder) 2012-02-16 15:48:35 PST
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.
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2012-02-16 15:51:22 PST
Will this require and/or check for a fixed or minimum version of LLVM?
Comment 5 Christian Holler (:decoder) 2012-02-16 15:54:06 PST
(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 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-22 14:05:11 PST
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.
Comment 7 Christian Holler (:decoder) 2012-02-23 07:36:05 PST
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.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-23 07:39:28 PST
Comment on attachment 600016 [details] [diff] [review]
Updated patch

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

r=me on the build stuff
Comment 9 Christian Holler (:decoder) 2012-02-23 07:39:59 PST
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.
Comment 10 Kai Engert (:kaie) 2012-02-23 07:47:22 PST
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.
Comment 11 Wan-Teh Chang 2012-03-06 16:02:38 PST
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.
Comment 12 Christian Holler (:decoder) 2012-03-06 16:09:52 PST
(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 :)
Comment 13 Christian Holler (:decoder) 2012-03-07 04:14:20 PST
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.
Comment 14 Christian Holler (:decoder) 2012-03-07 15:05:07 PST
Comment on attachment 603671 [details] [diff] [review]
Updated patch

Keeping r+ from first patch, as discussed on IRC.
Comment 15 Gary Kwong [:gkw] [:nth10sd] 2012-03-07 15:13:16 PST
Comment on attachment 603671 [details] [diff] [review]
Updated patch

http://hg.mozilla.org/integration/mozilla-inbound/rev/ad79ffdf94a3
Comment 16 Ed Morley [:emorley] 2012-03-08 13:55:55 PST
https://hg.mozilla.org/mozilla-central/rev/ad79ffdf94a3

Note You need to log in before you can comment on or make changes to this bug.