1083464 - Using JS::CompileOptions results in stack corruption

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladislav Samsonov]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36

Steps to reproduce:

I'm using Spidermonkey 33 and Clang 3.4 compiler. When I run following simple code it crashes with segmentation fault

int main(int argc, char* argv[]) {
	JS_Init();
	JSRuntime * rt = JS_NewRuntime(JS::DefaultHeapMaxBytes);
	if (!rt)
		return 1;
	JSContext * ctx = JS_NewContext(rt, 1024*8);
	if (!ctx)
		return 2;

	JS::CompileOptions op(ctx); // stack corruption here

	JS_DestroyRuntime(rt);
	JS_ShutDown();
	return 0;
}

I did some research with debugger and figured out that most likely constructor of CompileOptions corrupting stack when it is run from main thread. 

Spidermonkey itself compiled with gcc 4.8.2. Segfault only appears when the program above compiles with clang. However, gcc shows some signs of stack corruption too, but program runs just fine (well... not really; i meant - without segfault).

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Spidermonkey itself compiled with gcc 4.8.2

Are you sure that your gcc and your clang are ABI-compatible?

Comment 2

[Vladislav Samsonov]

(In reply to Boris Zbarsky [:bz] from comment #1)
> > Spidermonkey itself compiled with gcc 4.8.2
> 
> Are you sure that your gcc and your clang are ABI-compatible?

At least LLVM documentation says so. I've never had any problems with linking gcc-compiled libraries to clang-compiled code, I'm using clang for 4 years.
This may be the actual source of problems. So I will try to compile spidermonkey with clang anyway and I will post the result here... But nothing will change, probably.

Comment 3

[Vladislav Samsonov]

I compiled spidermonkey with clang, segfault still appears.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Thank you for checking that.

Next question: When compiling SpiderMonkey, is DEBUG defined?  What about when compiling your executable?

Comment 5

[Vladislav Samsonov]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Thank you for checking that.
> 
> Next question: When compiling SpiderMonkey, is DEBUG defined?

DEBUG is not defined. I'm compiling spidermonkey with this command:
./configure --enable-gold --without-intl-api && make - for GCC
./configure --without-intl-api && make - for clang

>  What about when compiling your executable?
Both debug and release builds have this problem.

Comment 6

[Vladislav Samsonov]

I figured out the source of this bug. When you build spidermonkey with ./configure script, --enable-exact-rooting option is enabled by default. So, the library is building with defined JSGC_USE_EXACT_ROOTING. However, when you use JS API from your program, there is no definition of JSGC_USE_EXACT_ROOTING anywhere. #define should have been added to js-config.h after building, but for some reason this doesn't happen for current version of spidermonkey. If i manually define JSGC_USE_EXACT_ROOTING, everything is fine. If i disable exact rooting in ./configure, everything is fine.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Vladislav, thank you for hunting that down...

Jason, do you know why this is not ending up in js-config.h?

Comment 8

[Jason Orendorff [:jorendorff]]

Good detective work, Vladislav.

Terrence, would you please take a look? From what I recall, any #define used in any public header needs to be in js-config.h, which is generated at configure time. It should be pretty clear how to do this from the existing examples.

Comment 9

[Jason Orendorff [:jorendorff]]

Vladislav, another possible problem with this code is that the JS::CompileOptions *destructor* is called after the runtime is destroyed. That's bad.

However it wouldn't cause the problem you're seeing, if I understand correctly---it would cause heap corruption on exit from main.

Comment 10

[Terrence Cole [:terrence]]

Attachment: bug_1083464-v0.diff

JSGC_USE_EXACT_ROOTING is long gone, as we no longer support other configurations. This is still an issue for JSGC_GENERATIONAL and JS_GC_SMALL_CHUNK_SIZE, so here is a patch.

Comment 11

[Vladislav Samsonov]

(In reply to Terrence Cole [:terrence] from comment #10)
> Created attachment 8510594 [details] [diff] [review]
> bug_1083464-v0.diff
> 
> JSGC_USE_EXACT_ROOTING is long gone, as we no longer support other
> configurations. This is still an issue for JSGC_GENERATIONAL and
> JS_GC_SMALL_CHUNK_SIZE, so here is a patch.

But why it is enabled but default? I still have to manually define JSGC_USE_EXACT_ROOTING, because there are blocks of code under #ifdef-s in header files which cause problems. Should i just build it with --disable-exact-rooting to bypass this?
Thank you for the answer.

Comment 12

[Terrence Cole [:terrence]]

(In reply to Vladislav Samsonov from comment #11)
> (In reply to Terrence Cole [:terrence] from comment #10)
> > Created attachment 8510594 [details] [diff] [review]
> > bug_1083464-v0.diff
> > 
> > JSGC_USE_EXACT_ROOTING is long gone, as we no longer support other
> > configurations. This is still an issue for JSGC_GENERATIONAL and
> > JS_GC_SMALL_CHUNK_SIZE, so here is a patch.
> 
> But why it is enabled but default? I still have to manually define
> JSGC_USE_EXACT_ROOTING, because there are blocks of code under #ifdef-s in
> header files which cause problems. Should i just build it with
> --disable-exact-rooting to bypass this?
> Thank you for the answer.

We removed it very recently very recently, so it's not in SM33. The attached patch is for tip; we would need to make a new patch for SM33 and request uplift separately. That said, this is pretty unlikely to get uplifted as it's not a sec-issue and doesn't impact firefox directly.

Comment 13

[Jeff Walden [:Waldo]]

"doesn't impact Firefox directly" is actually a reason it *would* be uplifted, usually.  It's no guarantee, but for a sufficiently small, non-invasive, easily-understood patch, it might well be taken.  Particularly if this affects the release candidates we're peddling now, and that matters to enough people, we should seriously think about taking a backported patch of some sort for this.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8510594 [details] [diff] [review]
bug_1083464-v0.diff

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

While we're in here:

1.  JSGC_GENERATIONAL appears in b2g/confvars.sh and browser/confvars.sh ... surely that is obsolete, right? Can we kill it off?

2.  Likewise, I think this means we can delete all mention of JSGC_GENERATIONAL and many of its friends from the root configure.in. If we can, we should. Redundant code sucks. (People building with these options shouldn't be affected: unrecognized configure options are blindly passed from the root configure to js/src/configure.)

Comment 15

[Terrence Cole [:terrence]]

Attachment: bug_1083464-v1.diff

https://tbpl.mozilla.org/?tree=Try&rev=df4a18d87de8

We have to use export in confdefs.sh to get the vars to the sub-configure, but that should be totally fine. With that change, it looks like we can kill off all of the duplication in the base configure.in.

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 8512201 [details] [diff] [review]
bug_1083464-v1.diff

This is a pretty big configure change, so I'm asking for an additional build peer review.

Comment 17

[Terrence Cole [:terrence]]

Attachment: bug_1083464-vr+.diff

Rebased and updated checking message.

Comment 18

[Terrence Cole [:terrence]]

Try builds are green, modulo unrelated bustage that was on tip at the time.
https://tbpl.mozilla.org/?tree=Try&rev=df4a18d87de8

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d83bcbd7e83c

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d83bcbd7e83c

Comment 21

[Terrence Cole [:terrence]]

[Tracking Requested - why for this release]:

Comment 22

[Terrence Cole [:terrence]]

Comment on attachment 8516255 [details] [diff] [review]
bug_1083464-vr+.diff

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Without this patch, it is difficult to correctly build a non-gecko project depending on SpiderMonkey. Since SpiderMonkey cuts its releases out of ESR, we need this patch uplifted before we can cut a decent SpiderMonkey release.

User impact if declined: A terrible SpiderMonkey release.

Fix Landed on Version: 36.

Risk to taking this patch (and alternatives if risky):
  Zero. This is only a build system change and only impacts building a non-gecko SpiderMonkey embedding project.

String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for esr31. Also, is this wontfix for 34/35?

Comment 24

[Terrence Cole [:terrence]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> Needs rebasing for esr31. Also, is this wontfix for 34/35?

Yes, I think so.

Comment 25

[Terrence Cole [:terrence]]

Attachment: fix_jsgc_exports_esr31_uplift-v0.diff

Uplifted patch for esr31. This version only had exact rooting, so there's much less to do.

Comment 26

[Terrence Cole [:terrence]]

Comment on attachment 8518957 [details] [diff] [review]
fix_jsgc_exports_esr31_uplift-v0.diff

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

I'm not sure what to put in the whiteboard to request checkin to an esr branch.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8518957 [details] [diff] [review]
fix_jsgc_exports_esr31_uplift-v0.diff

https://hg.mozilla.org/releases/mozilla-esr31/rev/15d1c2a4ea55

Comment 28

[Terrence Cole [:terrence]]

Back in the late 20's timeframe we flipped the default to ON for the shell for testing in the assumption that the other (incorrect) infra we added for browser builds would flip it back. Thus, removing the incorrect infra enabled exact rooting on mobile by accident. I've simply flipped the shell default back to what was expected by the browser.

https://hg.mozilla.org/releases/mozilla-esr31/rev/09ce3d77d902

Comment 29

[Terrence Cole [:terrence]]

Uh, no. Not possible.

Comment 30

[Terrence Cole [:terrence]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/09ce3d77d902
https://hg.mozilla.org/releases/mozilla-esr31/rev/d74ec3052a66

If I'm not careful, I'm going to wear out my clownshoes.

Comment 31

[Yves Gwerder]

Do I understand correclty that this patch disabled exact stack rooting by default in the ESR31 SpiderMonkey standalone release?
It would be good to land the release soon, because this will cause a lot of confusion otherwise (it already does). Packagers for Linux distributions already start creating packages because the current RC tarball hasn't changed since months and because applications (like 0 A.D.) will release versions with updated SpiderMonkey soon. However, if they pick the most recent code from here, they get different default settings than if they pick the most recent RC tarball. And as we know, these settings are essential and have to match the application settings.

Also I think the text in this commit is wrong:
https://hg.mozilla.org/releases/mozilla-esr31/rev/09ce3d77d902

"--enable-exact-rooting   Enable use of conservative stack scanning for GC]"
Enabling exact rooting actually disables the conservative stack scanning. So here "Enable" should be replaced by "Disable".

Comment 32

[Terrence Cole [:terrence]]

(In reply to Yves Gwerder from comment #31)
> Do I understand correclty that this patch disabled exact stack rooting by
> default in the ESR31 SpiderMonkey standalone release?
> It would be good to land the release soon, because this will cause a lot of
> confusion otherwise (it already does). Packagers for Linux distributions
> already start creating packages because the current RC tarball hasn't
> changed since months and because applications (like 0 A.D.) will release
> versions with updated SpiderMonkey soon. However, if they pick the most
> recent code from here, they get different default settings than if they pick
> the most recent RC tarball. And as we know, these settings are essential and
> have to match the application settings.
> 
> Also I think the text in this commit is wrong:
> https://hg.mozilla.org/releases/mozilla-esr31/rev/09ce3d77d902
> 
> "--enable-exact-rooting   Enable use of conservative stack scanning for GC]"
> Enabling exact rooting actually disables the conservative stack scanning. So
> here "Enable" should be replaced by "Disable".

Sorry, thought I had replied already:

No, the attached patch should not have changed the generated binary images at all. Exact rooting was enabled in both the shell and browser in 31, so any build made in that timeframe from either makefile should be compatible. Of course, that does not change the inevitable crash from the missing exports addressed by this patch, but hopefully any embedding would have noticed and worked around the missing defines locally already.