Using JS::CompileOptions results in stack corruption

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: vvladxx, Assigned: terrence)

Tracking

33 Branch
mozilla36
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, firefox-esr31 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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).
Keywords: 64bit
> Spidermonkey itself compiled with gcc 4.8.2

Are you sure that your gcc and your clang are ABI-compatible?
(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.
I compiled spidermonkey with clang, segfault still appears.
Thank you for checking that.

Next question: When compiling SpiderMonkey, is DEBUG defined?  What about when compiling your executable?
(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.
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.
Vladislav, thank you for hunting that down...

Jason, do you know why this is not ending up in js-config.h?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(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.
Flags: needinfo?(jorendorff) → needinfo?(terrence)
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.
Posted patch bug_1083464-v0.diff (obsolete) — Splinter Review
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.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Flags: needinfo?(terrence)
Attachment #8510594 - Flags: review?(jorendorff)
(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.
(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.
"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 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.)
Attachment #8510594 - Flags: review?(jorendorff) → review+
Posted patch bug_1083464-v1.diff (obsolete) — Splinter Review
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.
Attachment #8510594 - Attachment is obsolete: true
Attachment #8512201 - Flags: review+
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.
Attachment #8512201 - Flags: review?(ted)
Attachment #8512201 - Flags: review+
Attachment #8512201 - Flags: feedback+
Attachment #8512201 - Flags: review?(ted) → review+
Rebased and updated checking message.
Attachment #8512201 - Attachment is obsolete: true
Attachment #8516255 - Flags: review+
Try builds are green, modulo unrelated bustage that was on tip at the time.
https://tbpl.mozilla.org/?tree=Try&rev=df4a18d87de8
Keywords: 64bitcheckin-needed
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/d83bcbd7e83c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
[Tracking Requested - why for this release]:
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.
Attachment #8516255 - Flags: approval-mozilla-esr31?
Attachment #8516255 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Needs rebasing for esr31. Also, is this wontfix for 34/35?
Flags: needinfo?(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.
Flags: needinfo?(terrence)
Uplifted patch for esr31. This version only had exact rooting, so there's much less to do.
Attachment #8518957 - Flags: review+
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.
Attachment #8518957 - Flags: checkin?(ryanvm)
Comment on attachment 8518957 [details] [diff] [review]
fix_jsgc_exports_esr31_uplift-v0.diff

https://hg.mozilla.org/releases/mozilla-esr31/rev/15d1c2a4ea55
Attachment #8518957 - Flags: checkin?(ryanvm) → checkin+
Depends on: 1095628
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
Uh, no. Not possible.
No longer depends on: 1095628
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".
(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.
Duplicate of this bug: 1005793
Duplicate of this bug: 1027815
You need to log in before you can comment on or make changes to this bug.