294195 - Special regular expression (regexp) crashes browser

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Krzysztof Szafranek]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

Using substr() method on string containing references to elements matched by
regular expression crashes Firefox.

Reproducible: Always

Steps to Reproduce:
1. Call replace() method on a string in JS script in a similar way as shown on
test page
Actual Results:  
Browser crashes

Expected Results:  
Definitely not crash.

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050514
Firefox/1.0+ ID:2005051410
Crashes for me.
TalkBackIDs:
- TB5834667X [@ js_ValidContextPointer]
- TB5834771X [@ MSVCRT.DLL]

Comment 2

[Steve England [:stevee]]

Attachment: reporter's testcase

Comment 3

[Steve England [:stevee]]

Some more talkbackids
- TB5834926X [@ nsPropertyTable::DeleteAllPropertiesFor]
- TB5834989Q [@ MSVCRT.DLL]
- TB5835029Z [@ ntdll.dll]

Comment 4

[Erik Fabert]

The even shorter testcase

"".replace(/()/, "$1".slice(0,1))

generates an assertion in the debug shell:
Assertion failure: nbytes != 0, at jsapi.c:1494

Comment 5

[Bob Clary [:bc] (inactive)]

Checking in regress-294195-01.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-294195-01.js,v  <-- 
regress-294195-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-294195-02.js,v
done
Checking in regress-294195-02.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-294195-02.js,v  <-- 
regress-294195-02.js
initial revision: 1.1

Comment 6

[Jan Darmochwal]

AFAICT the problem is that interpret_dollar() in js/src/jsstr.c depends on
JSSTRING_CHARS(repstr) ending with a NUL char. This seems not to true if repstr
is a dependent string (like with string.slice()).

In the testcase jschar *dp in interpret_dollar() points to the last (only)
character in repstr ("$"). interpret_dollar() will then happily look at the next
character ("1") and interpret the "$" depending on that next character although
that character does not belong to repstr.

Comment 7

[Brendan Eich [:brendan]]

Yes, thanks -- will fix ASAP.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Apologies for any stylistic faux pas in advance. This makes interpret_dollar
not assume null-termination.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

This just makes things a bit more C-like.

Comment 10

[Jan Darmochwal]

(In reply to comment #9)

>+            while (++cp != ep && JS7_ISDEC(dc = *ep)) {

That should probably be *cp instead of *ep.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v3

Yikes! Thanks for pointing that out.

Comment 12

[Bryan]

Hi,
Can someone please check the patcgh so we can get shipped out?
This is taking too long.

Comment 13

[timeless]

[e-mail]: no.
we have a process.
the first step is reporting the bug (done).
the second step is analyzing the bug (done).
the third step is writing and posting a correct a patch (maybe done).
the fourth step is asking for and receiving a positive review (not done!).
after all reviews are gathered (not done), if the tree is frozen (it is), then
you need to get approval (not done).
lastly, someone with cvs access to the partition needs to commit it (clearly not
done), i'm not committing a patch that doesn't have the necessary reviews and
approvals, it's bad for the safety of my cvs account.
patience is a virtue, please practice it.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 295734 has been marked as a duplicate of this bug. ***

Comment 15

[Uri Bernstein]

Adding "regexp" to summary for easier searching

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 184537 [details] [diff] [review]
patch v3

>@@ -1306,23 +1311,29 @@ interpret_dollar(JSContext *cx, jschar *
>                 return NULL;
> 
>             /* Check for overflow to avoid gobbling arbitrary decimal digits. */
>             num = 0;
>             cp = dp;
>-            while ((dc = *++cp) != 0 && JS7_ISDEC(dc)) {
>+            while (++cp != ep && JS7_ISDEC(dc = *cp)) {

Robustness nit: I prefer < rather than !=, in case by some off-by-one or
insane-caller bug cp > ep.

>+
>+            /* 
>+             * Note: cp cannot be greater than ep because of the dp + 1 == ep
>+             * check above.
>+             */

JS_ASSERT?  Beats a comment every time.  Could comment and assert; often the
assertion speaks for itself.

>             cp = dp + 2;
>-            dc = *cp;
>-            if ((dc != 0) && JS7_ISDEC(dc)) {
>+            dc = (cp == ep) ? 0 : *cp;
>+
>+            if (dc != 0 && JS7_ISDEC(dc)) {

This reintroduces a NUL sensitivity that may not matter, but it seems better
and clearer to me to do:

	    if (cp < ep && (dc = *cp, JS7_ISDEC(dc))) {

Note righteous nested comma operator usage.

One more patch and I'll r+a=.

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v4

This fixes up all of the review comments. I did away with the comment and the
assert by making the code not rely on the comment anymore (changing != to <
everywhere).

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 185586 [details] [diff] [review]
patch v4

r+a=me, thanks for fixing this.

/be

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.