312069 - crash in js1_5/Array/regress-157652.js [@ JS_malloc]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

regressed between 2005-10-09 and 2005-10-10 on trunk and between 2005-10-10 and
2005-10-11 on branch

NTDLL! 7c901230()
JS_malloc(JSContext * 0x00032dd8, unsigned int 0) line 1589 + 28 bytes
array_sort(JSContext * 0x00032dd8, JSObject * 0x00039598, unsigned int 0, long *
0x0041dabc, long * 0x0013e568) line 936 + 13 bytes
js_Invoke(JSContext * 0x00032dd8, unsigned int 0, unsigned int 0) line 1163 + 23
bytes
js_Interpret(JSContext * 0x00032dd8, unsigned char * 0x00426887, long *
0x0013ee24) line 3486 + 15 bytes
js_Execute(JSContext * 0x00032dd8, JSObject * 0x000387c8, JSScript * 0x004267c8,
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0013fecc) line 1393 + 19 bytes
JS_ExecuteScript(JSContext * 0x00032dd8, JSObject * 0x000387c8, JSScript *
0x004267c8, long * 0x0013fecc) line 4009 + 25 bytes
Process(JSContext * 0x00032dd8, JSObject * 0x000387c8, char * 0x00032d31) line
223 + 22 bytes
ProcessArgs(JSContext * 0x00032dd8, JSObject * 0x000387c8, char * * 0x00032cc4,
int 4) line 426 + 23 bytes
main(int 4, char * * 0x00032cc4, char * * 0x00033248) line 2564 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 7c816d4f()

Comment 1

[Igor Bukanov]

Attachment: Fix

Fixing own fault in not following a simple rule: when checking for overflow
make sure that unknown quantity stays alone on one side of a compare operation.


So to follow that rule I changed original cast to double to 
len < compile_time_limit.

Comment 2

[Brendan Eich [:brendan]]

Must-fix for 1.8.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 199198 [details] [diff] [review]
Fix

>     /*
>-     * Memory for temporary array incliding one extra jsval as working space
>-     * for js_HeapSort.
>+     * We need temporary array for len jsval to hold elements of JS array,

Nit: "We need *a* temporary array".

>+     * check that its size does not overflow size_t which could lead to
>+     * indexing beyond the end of the malloc'd vector.
>      */

Cool, a local root beats adding 1 to len.  Thanks, Igor.  Are you willing to
check this in on the trunk?  You have the CVS access already.  I'll get it
branch approval.

/be

Comment 4

[Igor Bukanov]

Attachment: Fix 2

Unfortunately "Fix 1" was too quick: I forgot to remove "+1" from the allocated
array rooting code. Hopefully a morning fix is better then late evening one.

Comment 5

[Igor Bukanov]

(In reply to comment #3)
> Nit: "We need *a* temporary array".

Those articles in English, will I ever learn how to use them properly? Russian
has no notion of such things and although there are so called rules about their
usage, I saw too many cases (not only on Slashdot ;) when they were not
followed. Or perhaps those rules are not real ones? Once I picked up a book
claiming to contain *all* the rules about English articles, but then there were
150 pages in the book. So I just wonder how children picks on their own from
parents all the 150 pages of complex linguistic rules?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 199268 [details] [diff] [review]
Fix 2

Taking the liberty of giving r+ here. I honestly was wondering about this when
reading the last patch, but didn't speak up.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 199268 [details] [diff] [review]
Fix 2

>+     * We need a temporary array for len jsval to hold elements of JS array,
>+     * check that its size does not overflow size_t which could lead to
>+     * indexing beyond the end of the malloc'd vector.

More English nits. addresed by this revision:

     * We need a temporary array of len jsvals to hold elements of the array.
     * Check that its size does not overflow size_t, which would allow for
     * indexing beyond the end of the malloc'd vector.


>-    fp->nvars = len + 1;
>+    fp->nvars = len;

D'oh -- I should have caught that.

Looks good with nit picked.  Igor, if you land this on the trunk now, it will
help us to get it onto the branch.  Thanks,

/be

Comment 8

[Igor Bukanov]

Attachment: Patch to commit: Fix 2 + English language nits

Comment 9

[Igor Bukanov]

(In reply to comment #7)
> if you land this on the trunk now, it will
> help us to get it onto the branch.

Done

Comment 10

[Asa Dotzler [:asa]]

What's this fixing, what's the risk, and what kind of testing has been done. Has
there been a testcase verified on the trunk yet?

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #10)
> What's this fixing, what's the risk, and what kind of testing has been done. Has
> there been a testcase verified on the trunk yet?

This is fixing a crash regression which was caused earlier this week. I am in
the process of generating my set of optimized and debugs builds for 1.5 and
trunk and will kick off the javascript test library shortly. They will complete
sometime before the end of the day. 

Comment 12

[Brendan Eich [:brendan]]

Asa: this is a off-by-one crash regression from a security fix just checked into
the branch.  It's in the trunk now, so we can verify as Bob says, but the odds
of not shipping this fix in 1.8 / fx1.5 are zero.

/be

Comment 13

[Scott MacGregor]

marking this as fixed since it's fixed on the trunk.

Once Bob verifies (sounds like he's all over it), we'll look into branch approval. 

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

Queueing up.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

trunk looks ok with no regressions I can find so far.

Comment 16

[Bob Clary [:bc] (inactive)]

(In reply to comment #15)
> trunk looks ok with no regressions I can find so far.

I take this back until the test results in the original bug are understood.

Comment 17

[Asa Dotzler [:asa]]

Bob, what more testing needs to happen (if any) before we approve this for the
branch?

Comment 18

[Brendan Eich [:brendan]]

There's an old latent bug (over 8 years old) that is biting the fix patched
here. The fix is good once we fix the latent bug. See bug 311497.

/be

Comment 19

[Scott MacGregor]

311497 is now on the branch. Are we ready to land this now? 

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH.

Comment 21

[Bob Clary [:bc] (inactive)]

no crash firefox 1.5 rc2 winxp/linux

Comment 22

[Mike Schroepfer]

Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

Bkap needs approval to land for 1.7 trees

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

a=dveditz

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

This was checked into the 1.7 branch along with my checkin for bug 311497.

Comment 25

[Bob Clary [:bc] (inactive)]

verified no crash in firefox/1.7.13/20060216 win/linux/mac, no crash on 1.8, 1.8.0.1, 1.9a1 win/linux/mac. Note that firefox/1.7.13 has "out of memory" while 1.8.0.1, 1.8 and 1.9a1 do not.

Comment 26

[Bob Clary [:bc] (inactive)]

v ff on 1.7.13 on windows/linux/mac, moz on 1.7.13 on windows.