Closed Bug 294195 Opened 15 years ago Closed 15 years ago

Special regular expression (regexp) crashes browser

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta3

People

(Reporter: ebraun, Assigned: mrbkap)

References

()

Details

(Keywords: crash, js1.5, testcase)

Attachments

(2 files, 3 obsolete files)

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.
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]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Keywords: testcase
Summary: Special regular regular expression crashes browser → Special regular expression crashes browser
Some more talkbackids
- TB5834926X [@ nsPropertyTable::DeleteAllPropertiesFor]
- TB5834989Q [@ MSVCRT.DLL]
- TB5835029Z [@ ntdll.dll]
The even shorter testcase

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

generates an assertion in the debug shell:
Assertion failure: nbytes != 0, at jsapi.c:1494
Keywords: crash, testcase
Summary: Special regular expression crashes browser → Special regular regular expression crashes browser
Keywords: crash, testcase
Summary: Special regular regular expression crashes browser → Special regular expression crashes browser
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
Flags: testcase+
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.
Yes, thanks -- will fix ASAP.

/be
Assignee: general → brendan
Flags: blocking1.8b2+
Keywords: js1.5
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Attached patch patch v1 (obsolete) — Splinter Review
Apologies for any stylistic faux pas in advance. This makes interpret_dollar
not assume null-termination.
Assignee: brendan → mrbkap
Status: NEW → ASSIGNED
Attachment #184021 - Flags: review?(brendan)
Attached patch patch v2 (obsolete) — Splinter Review
This just makes things a bit more C-like.
Attachment #184021 - Attachment is obsolete: true
Attachment #184519 - Flags: review?(brendan)
Attachment #184021 - Flags: review?(brendan)
(In reply to comment #9)

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

That should probably be *cp instead of *ep.
Attached patch patch v3 (obsolete) — Splinter Review
Yikes! Thanks for pointing that out.
Attachment #184519 - Attachment is obsolete: true
Attachment #184537 - Flags: review?(brendan)
Attachment #184519 - Flags: review?(brendan)
Hi,
Can someone please check the patcgh so we can get shipped out?
This is taking too long.
bryanwellander@socal.rr.com: 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.
Flags: blocking1.8b3+
Flags: blocking1.8b2-
Flags: blocking1.8b2+
*** Bug 295734 has been marked as a duplicate of this bug. ***
Adding "regexp" to summary for easier searching
Summary: Special regular expression crashes browser → Special regular expression (regexp) crashes browser
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
Attached patch patch v4Splinter Review
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).
Attachment #184537 - Attachment is obsolete: true
Attachment #185586 - Flags: review?(brendan)
Comment on attachment 185586 [details] [diff] [review]
patch v4

r+a=me, thanks for fixing this.

/be
Attachment #185586 - Flags: review?(brendan)
Attachment #185586 - Flags: review+
Attachment #185586 - Flags: approval1.8b3+
Attachment #184537 - Flags: review?(brendan)
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta2 → mozilla1.8beta3
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.