824856 - Crash [@ QuoteString] or [@ js_NewStringCopyN] or "Assertion failure: limit >= start,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

"xy".match(/((x)??){2}y/)

asserts js debug shell on m-c changeset f5ed2691d901 without any CLI arguments at Assertion failure: limit >= start,

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116360:cd2eb9705765
user:        Gary Kwong
date:        Mon Dec 17 16:57:48 2012 -0800
summary:     Workaround YARR assert (bug 808478, r=sstangl).

I forgot to set the actual patch author when I was helping to land the patch in bug 808478. :-/ The original patch author was dvander.

Prior to that changeset, the testcase was asserting at Assertion failure: (&term - term.atom.parenthesesWidth)->inputPosition == term.inputPosition, which bug 808478 worked around.


This seems to be asserting in our regex code instead of YARR code. The original assert was added in bug 673188. I'm not sure though.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

"2\"".match("(((2)??)+\")")()

crashes js opt shell on m-c changeset 0d771761b9b3 without any CLI arguments at QuoteString. Turning s-s and setting sec-critical because it seems to be accessing a weird memory address.

The incriminating changeset landed only in 20, but that was a workaround, and the original purported regression (upgrading YARR) landed in 19, so setting flags accordingly.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for testcase in comment 1

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

"\u66d6J".split(/((\u66d6)??){7}J/)

is another testcase that crashes js opt shell at memmove$VARIANT$sse42 with js_NewStringCopyN on the stack on m-c changeset dec6aa71da64.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for testcase in comment 3

The testcase in comment 3 also has the same assertion, and also seems to be accessing a weird memory address 0xfffa5a00.

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Gary Kwong [:gkw] from comment #1)
> "2\"".match("(((2)??)+\")")()
> 
> crashes js opt shell on m-c changeset 0d771761b9b3 without any CLI arguments
> at QuoteString. Turning s-s and setting sec-critical because it seems to be
> accessing a weird memory address.
> 
> The incriminating changeset landed only in 20, but that was a workaround,
> and the original purported regression (upgrading YARR) landed in 19, so
> setting flags accordingly.

Can we mark this bug with sg:crit or sg:high prior to tracking? It's not clear how we're rating this internally.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Can we mark this bug with sg:crit or sg:high prior to tracking? It's not
> clear how we're rating this internally.

> crashes js opt shell on m-c changeset 0d771761b9b3 without any CLI arguments
> at QuoteString. Turning s-s and setting sec-critical because it seems to be
> accessing a weird memory address.

Oops, I missed out setting the sec-critical keyword. Added it now.

Comment 7

[Alex Keybl [:akeybl]]

David - this is an sg:crit JS regression in FF19. Can you help find an owner?

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

This was introduced with the Yarr import, which I did, so I'll take it.

Comment 9

[Alex Keybl [:akeybl]]

(In reply to David Anderson [:dvander] from comment #8)
> This was introduced with the Yarr import, which I did, so I'll take it.

Any updates? We're now coming up on beta 4, and this is a FF19 regression. We don't have much time to resolve.

Comment 10

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unknown exception (check manually)

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Initial analysis is that this was introduced in https://bugs.webkit.org/show_bug.cgi?id=73728 (rev 108858). There are some subtraction expressions that were casually swapped, and this causes problems for certain inputs (i.e. 0 - -1 == 1, instead of -1 - 0 == -1).

I have no idea what the right fix for this is. I don't have access to the WebKit bug that made these changes, so maybe there was another underlying security issue. We could (1) revert the expressions back to what was in Firefox 18, since we don't know of any outstanding problems there. We could (2) revert just the ones causing this particular bug. We could (3) make these regular expressions throw a JavaScript exception, but that might break the web.

I'll try-run a patch for (2) and see what happens.

Filed WebKit bug @ https://bugs.webkit.org/show_bug.cgi?id=107898

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

verified to fix the test cases in this bug

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Update: that idea totally failed on tbpl. Back to the drawing board.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Gary, I think we should backout the original patch, and disable the assert. Any thoughts on this? It doesn't seem like there is a security issue when the assert fires - for all we know it could be bogus, and fuzzing opt builds will tell us if/when there is a serious problem.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to David Anderson [:dvander] from comment #14)
> Gary, I think we should backout the original patch, and disable the assert.
> Any thoughts on this? It doesn't seem like there is a security issue when
> the assert fires - for all we know it could be bogus, and fuzzing opt builds
> will tell us if/when there is a serious problem.

Sure, let's try that. Shall we also add the testcases in this bug while we're at it?

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

This reverts the previous patch, but leaves the ASSERT disabled.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 714659 [details] [diff] [review]
fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

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?
If not all supported branches, which bug introduced the flaw?

Regressing cset landed on m-c on Dec 17 and no prior branches.

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

This patch will apply.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 714659 [details] [diff] [review]
fix

Sec-approval+. It would have been nice to have gotten this into 19 since then we would never have shipped this issue.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I have verified that the patch here fixes the testcase in bug 828019 as well.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e5f82315a79

Comment 21

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This was landed.

http://hg.mozilla.org/mozilla-central/rev/4e5f82315a79

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's get this nominated for uplift in that case, with a risk assessment, if we're going to take this in FF20 we really would want to have it landed before Tuesday's go to build on beta5.

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 714659 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 808478
User impact if declined: possible sg:crit
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 714659 [details] [diff] [review]
fix

low risk sec-crit regression, approving for uplift.
Please land by tonight(3/11) or early morning PT  to get it into our next beta.

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-aurora/rev/173181c18cf0
https://hg.mozilla.org/releases/mozilla-beta/rev/9ec0e2243b0a