288688 - JS "lambda" replace exposes malloc heap space after end of JS string

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Quick and dirty patch next.  There's a better patch that should be done, but the
patch I'll attach next fixes this, AFAICT.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: quick and dirty fix

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

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: 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.

/be

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

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

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

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: testcase

preserving testcase for the future in case the cubic.xfo.org.ru blog ever goes
away.

Comment 6

[Brendan Eich [:brendan]]

BTW, this bug is like 8+ years old.  Roger Lawrence fixed half of it in 2000:

r=norris,waldemar
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.

/be

Comment 7

[Brendan Eich [:brendan]]

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.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix, v3

Comment 9

[Brendan Eich [:brendan]]

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.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix, v4

Comment 11

[Brendan Eich [:brendan]]

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 ;-).

/be

Comment 12

[Bob Clary [:bc] (inactive)]

js/tests/js1_5/Regress/regress-288688.js checked in.

Comment 13

[Juha-Matti Laurio]

This is assigned to SA14821 (including Suite 1.7.6)
http://secunia.com/advisories/14821/

and SA14820 (Firefox 1.0.1 and 1.0.2)
http://secunia.com/advisories/14820/

Link to "Mozilla Products Arbitrary Memory Exposure Test" page:
http://secunia.com/mozilla_products_arbitrary_memory_exposure_test/

Comment 14

[Mike Kaply [:mkaply]]

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

Comment 15

[Matthias Versen [:Matti]]

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

Comment 16

[Mike Kaply [:mkaply]]

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

Comment 17

[Steve England [:stevee]]

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

Comment 18

[seventhguardian_]

This bug is not entirely solved!!

If you go to the secunia site
http://secunia.com/mozilla_products_arbitrary_memory_exposure_test

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] (inactive)]

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]

(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] (inactive)]

(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] (inactive)]

verified fixed.