Closed Bug 1109889 Opened 5 years ago Closed 5 years ago

Crash [@ ??] with gczeal and recursion

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 + verified
firefox36 + verified
firefox37 + verified
firefox-esr31 35+ fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main35+][adv-esr31.4+][b2g-adv-main2.2+])

Crash Data

Attachments

(6 files)

The following testcase crashes on mozilla-central revision d7c76fe69e9a (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --enable-debug, run with --no-threads --fuzzing-safe):

var g = newGlobal();
gczeal(2, 1)
function recurse(x) {
	recurse(g.f == this == 3);
};
recurse(0)



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0xf6a9026f in ?? ()
#0  0xf6a9026f in ?? ()
#1  0xffffff82 in ?? ()
eax	0x0	0
ebx	0xffefe2b4	-1056076
ecx	0xffffc5cc	-14900
edx	0x0	0
esi	0xffefe29c	-1056100
edi	0xf6b54ce0	-155890464
ebp	0xffefe2b8	4293911224
esp	0xffefe260	4293911136
eip	0xf6a9026f	4138271343
=> 0xf6a9026f:	cmp    (%eax),%eax
   0xf6a90271:	add    %al,(%eax)


Marking s-s because this crash involves GC and is on the heap.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8792056f152c
user:        Jan de Mooij
date:        Thu Nov 13 17:39:51 2014 +0100
summary:     Bug 1093573 part 13 - Handle closing legacy generators correctly. r=wingo,shu

This iteration took 579.552 seconds to run.
Jan, is bug 1093573 a likely regressor?
Blocks: 1093573
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] GMT+8 Dec 12 - 22 from comment #2)
> Jan, is bug 1093573 a likely regressor?

For this testcase it is, but the underlying problem is older. Very nasty and serious bug:

(1) We have an Ion IC stub that calls the exception handler (after it does a callWithABI that fails).
(2) HandleException calls EnsureExitFrame for each frame.
(3) HandleException triggers a GC.
(4) Due to (2), the GC does not mark the Ion IC stub code and it's freed.
(5) HandleException returns to garbage.

This might be responsible for some of our JIT top crashes, not sure.

I'm working on a patch that moves the call to the exception handler to the "exception tail" JitCode. That fixes the problem because this thing is never collected. It's also nicer because we generate less JIT code.
Keywords: sec-critical
Attached patch PatchSplinter Review
Moves the callWithABI to the shared exception tail stub as described in comment 3.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8536760 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8536760 [details] [diff] [review]
Patch

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

Nice patch! It does not bring any attention to the source of the error.
Attachment #8536760 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8536760 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easy, problem is not obvious from the patch.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
Assuming all branches.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely; safe patch.
Attachment #8536760 - Flags: sec-approval?
[Tracking Requested - why for this release]:

Sec bug, might fix some JIT crashes.
Comment on attachment 8536760 [details] [diff] [review]
Patch

sec-approval+ for trunk.

Once it is in, let's get patches nominated for Aurora, Beta, and ESR31.
Attachment #8536760 - Flags: sec-approval? → sec-approval+
needinfo myself to post branch patches.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/2a2e04192d58
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8536760 [details] [diff] [review]
Patch

This patch should apply against aurora with minimal conflicts, will post patches for beta and other branches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown, older JIT bug.
User impact if declined: Security bugs, crashes.
Testing completed: On m-c and m-i.
Risk to taking this patch (and alternatives if risky): Low risk, safe patch.
String or UUID changes made by this patch: None.
Flags: needinfo?(jdemooij)
Attachment #8536760 - Flags: approval-mozilla-esr31?
Attachment #8536760 - Flags: approval-mozilla-beta?
Attachment #8536760 - Flags: approval-mozilla-b2g34?
Attachment #8536760 - Flags: approval-mozilla-b2g32?
Attachment #8536760 - Flags: approval-mozilla-b2g30?
Attachment #8536760 - Flags: approval-mozilla-aurora?
Attached patch Patch for betaSplinter Review
Attachment #8537837 - Flags: review+
Attached patch Patch for b2g34Splinter Review
Attachment #8537838 - Flags: review+
Attached patch Patch for b2g32Splinter Review
Attachment #8537840 - Flags: review+
Comment on attachment 8536760 [details] [diff] [review]
Patch

B2G sec-crits have auto-approval to land once approved for affected Firefox branches.
Attachment #8536760 - Flags: approval-mozilla-b2g34?
Attachment #8536760 - Flags: approval-mozilla-b2g32?
Attachment #8536760 - Flags: approval-mozilla-b2g30?
Attached patch Patch for ESR31Splinter Review
Attachment #8537843 - Flags: review+
Attached patch Patch for b2g30Splinter Review
Conflicts on all branches, most of them were pretty trivial to fix but it took some time.
Attachment #8537844 - Flags: review+
Attachment #8536760 - Flags: approval-mozilla-esr31?
Attachment #8536760 - Flags: approval-mozilla-esr31+
Attachment #8536760 - Flags: approval-mozilla-beta?
Attachment #8536760 - Flags: approval-mozilla-beta+
Attachment #8536760 - Flags: approval-mozilla-aurora?
Attachment #8536760 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main35+][adv-esr31.4+]
Reproduced the original issue several times using the steps from comment #0 with changeset d7c76fe69e9a. Once reproduced, verified with the following builds:

* m-c using changeset d480b3542cc2 - PASSED
* m-a using changeset bd53ccf7ce3e - PASSED
* m-b using changeset 3129d08edd20 - PASSED
Whiteboard: [jsbugmon:update][adv-main35+][adv-esr31.4+] → [jsbugmon:update][adv-main35+][adv-esr31.4+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.