Last Comment Bug 288688 - JS "lambda" replace exposes malloc heap space after end of JS string
: JS "lambda" replace exposes malloc heap space after end of JS string
: fixed-aviary1.0.3, fixed1.7.7, js1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical with 1 vote (vote)
: mozilla1.8beta2
Assigned To: Brendan Eich [:brendan]
: 288608 288685 289134 289194 290161 290310 (view as bug list)
Depends on: 288818
Blocks: sbb+
  Show dependency treegraph
Reported: 2005-04-01 13:40 PST by Brendan Eich [:brendan]
Modified: 2007-03-22 23:00 PDT (History)
22 users (show)
brendan: blocking1.7.7+
brendan: blocking‑aviary1.0.3+
dveditz: blocking‑aviary1.5+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

quick and dirty fix (1.75 KB, patch)
2005-04-01 13:44 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
better fix (2.16 KB, patch)
2005-04-01 14:00 PST, Brendan Eich [:brendan]
brendan: superreview-
Details | Diff | Review
testcase (2.58 KB, text/html)
2005-04-01 16:24 PST, Daniel Veditz [:dveditz]
no flags Details
fix, v3 (2.42 KB, patch)
2005-04-01 18:23 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix, v4 (2.52 KB, patch)
2005-04-01 18:40 PST, Brendan Eich [:brendan]
shaver: review+
dbaron: superreview+
Details | Diff | Review

Description Brendan Eich [:brendan] 2005-04-01 13:40:07 PST
Quick and dirty patch next.  There's a better patch that should be done, but the
patch I'll attach next fixes this, AFAICT.

Comment 1 Brendan Eich [:brendan] 2005-04-01 13:44:09 PST
Created attachment 179311 [details] [diff] [review]
quick and dirty fix

Really, all of cx->regExpStatics should be stacked and unstacked around the
lambda invocations.

Comment 2 Brendan Eich [:brendan] 2005-04-01 14:00:38 PST
Created attachment 179313 [details] [diff] [review]
better fix

Lacking block scope and dynamically scoped variables, JS has never tried to
emulate Perl's dynamic scope for $1, $&, etc. with the pre-ECMA extensions
analogous to those vars (RegExp.$1, RegExp.lastMatch, etc).  Here's a case
where it must.

Comment 3 Brendan Eich [:brendan] 2005-04-01 14:05:50 PST
*** Bug 288608 has been marked as a duplicate of this bug. ***
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-01 14:12:51 PST
*** Bug 288685 has been marked as a duplicate of this bug. ***
Comment 5 Daniel Veditz [:dveditz] 2005-04-01 16:24:22 PST
Created attachment 179332 [details]

preserving testcase for the future in case the blog ever goes
Comment 6 Brendan Eich [:brendan] 2005-04-01 17:49:04 PST
BTW, this bug is like 8+ years old.  Roger Lawrence fixed half of it in 2000:

Fixes for bugs#23607, 23608, 23610, 23612, 23613. Also, first cut at URI
encode & decode routines.

Unfortunately, AFAICT none of the bugs he cites had anything to do with the two
hunks of that revision:

@@ -1061,16 +1080,22 @@ find_replen(JSContext *cx, ReplaceData *
@@ -1138,16 +1163,17 @@ find_replen(JSContext *cx, ReplaceData *

that half-fixed the original 1997-era bug.

Comment 7 Brendan Eich [:brendan] 2005-04-01 18:21:09 PST
Comment on attachment 179313 [details] [diff] [review]
better fix

dbaron shoots, he scores: cx->regExpStatics.moreParens points to malloc'ed
space that could be realloc'ed by the inner match.

Comment 8 Brendan Eich [:brendan] 2005-04-01 18:23:46 PST
Created attachment 179344 [details] [diff] [review]
fix, v3
Comment 9 Brendan Eich [:brendan] 2005-04-01 18:37:58 PST
Comment on attachment 179344 [details] [diff] [review]
fix, v3

I am a crispy critter right now.  This leaks the inner moreParens, if
allocated.  dbaron is two for two.

Comment 10 Brendan Eich [:brendan] 2005-04-01 18:40:27 PST
Created attachment 179347 [details] [diff] [review]
fix, v4
Comment 11 Brendan Eich [:brendan] 2005-04-01 18:53:26 PST
Fixed on trunk, AVIARY_1_0_1_20050124_BRANCH, and MOZILLA_1_7_BRANCH.

Thanks for the report, I hope that's the last bug from 1997 left ;-).

Comment 12 Bob Clary [:bc:] 2005-04-01 22:39:09 PST
js/tests/js1_5/Regress/regress-288688.js checked in.
Comment 13 Juha-Matti Laurio 2005-04-05 10:13:10 PDT
This is assigned to SA14821 (including Suite 1.7.6)

and SA14820 (Firefox 1.0.1 and 1.0.2)

Link to "Mozilla Products Arbitrary Memory Exposure Test" page:

Comment 14 Mike Kaply [:mkaply] (Out June 27-July 5) 2005-04-05 11:26:35 PDT
*** Bug 289134 has been marked as a duplicate of this bug. ***
Comment 15 Matthias Versen [:Matti] 2005-04-05 13:56:23 PDT
*** Bug 289194 has been marked as a duplicate of this bug. ***
Comment 16 Mike Kaply [:mkaply] (Out June 27-July 5) 2005-04-13 10:19:47 PDT
*** Bug 290161 has been marked as a duplicate of this bug. ***
Comment 17 Steve England [:stevee] 2005-04-14 06:48:41 PDT
*** Bug 290310 has been marked as a duplicate of this bug. ***
Comment 18 seventhguardian_ 2005-05-13 14:28:40 PDT
This bug is not entirely solved!!

If you go to the secunia site

and do the test with 1.0.2, it shows some contents of the memory.

Now with 1.0.3 (linux) it only shows some X's, but the entire system crashes
after doing this test a few times!!

Dont know if this is another problem, but here it is, since it's triggered by
the same thing..
Comment 19 Bob Clary [:bc:] 2005-05-13 14:48:30 PDT
This bug was fixed after Firefox 1.0.2. I just checked secunia with Firefox
1.0.4 and it does not show the 'XXX'. Please upgrade to 1.0.4 as it contains
significant security updates and retest.
Comment 20 Vladimir V. Perepelitsa 2005-05-13 14:58:08 PDT
(In reply to comment #19)
> 1.0.4 and it does not show the 'XXX'. 

Heh! It SHOULD show "XXX..." because it's correct behaviour of the script.
Comment 21 Bob Clary [:bc:] 2005-05-13 15:13:05 PDT
(In reply to comment #20)

> Heh! It SHOULD show "XXX..." because it's correct behaviour of the script.

You are right of course, doh! but it looks like a variant of bug 293940 prevents
the string from displaying. You can check that the mem string is 'XXX...X' by
setting a breakpoint at line 247 in the page where 
document.getElementById('result').value = mem;

Comment 22 Bob Clary [:bc:] 2006-08-19 11:12:43 PDT
verified fixed.

Note You need to log in before you can comment on or make changes to this bug.