Last Comment Bug 312069 - crash in js1_5/Array/regress-157652.js [@ JS_malloc]
: crash in js1_5/Array/regress-157652.js [@ JS_malloc]
Status: VERIFIED FIXED
: crash, regression, verified1.7.13, verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
http://lxr.mozilla.org/mozilla/source...
Depends on: 311497
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-11 11:05 PDT by Bob Clary [:bc:]
Modified: 2011-08-05 22:29 PDT (History)
6 users (show)
brendan: blocking1.8rc1+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (2.33 KB, patch)
2005-10-11 14:03 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Fix 2 (2.67 KB, patch)
2005-10-11 23:21 PDT, Igor Bukanov
brendan: review+
mrbkap: review+
Details | Diff | Splinter Review
Patch to commit: Fix 2 + English language nits (2.68 KB, patch)
2005-10-12 00:33 PDT, Igor Bukanov
brendan: review+
brendan: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description Bob Clary [:bc:] 2005-10-11 11:05:46 PDT
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 2005-10-11 14:03:39 PDT
Created attachment 199198 [details] [diff] [review]
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] 2005-10-11 17:43:19 PDT
Must-fix for 1.8.

/be
Comment 3 Brendan Eich [:brendan] 2005-10-11 17:45:50 PDT
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 2005-10-11 23:21:19 PDT
Created attachment 199268 [details] [diff] [review]
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 2005-10-11 23:51:43 PDT
(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) 2005-10-12 00:07:40 PDT
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] 2005-10-12 00:15:01 PDT
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 2005-10-12 00:33:14 PDT
Created attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits
Comment 9 Igor Bukanov 2005-10-12 00:40:48 PDT
(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] 2005-10-12 11:10:19 PDT
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:] 2005-10-12 11:30:42 PDT
(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] 2005-10-12 11:35:22 PDT
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 2005-10-12 18:57:22 PDT
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] 2005-10-12 19:50:25 PDT
Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

Queueing up.

/be
Comment 15 Bob Clary [:bc:] 2005-10-12 20:24:50 PDT
trunk looks ok with no regressions I can find so far.
Comment 16 Bob Clary [:bc:] 2005-10-12 20:56:02 PDT
(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] 2005-10-13 18:55:55 PDT
Bob, what more testing needs to happen (if any) before we approve this for the
branch?
Comment 18 Brendan Eich [:brendan] 2005-10-13 19:00:35 PDT
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 2005-10-17 14:30:25 PDT
311497 is now on the branch. Are we ready to land this now? 
Comment 20 Blake Kaplan (:mrbkap) 2005-10-17 15:10:19 PDT
Fix checked into MOZILLA_1_8_BRANCH.
Comment 21 Bob Clary [:bc:] 2005-11-09 19:34:23 PST
no crash firefox 1.5 rc2 winxp/linux
Comment 22 Mike Schroepfer 2006-02-10 19:04:08 PST
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] 2006-02-12 22:20:30 PST
Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

a=dveditz
Comment 24 Blake Kaplan (:mrbkap) 2006-02-14 18:31:24 PST
This was checked into the 1.7 branch along with my checkin for bug 311497.
Comment 25 Bob Clary [:bc:] 2006-02-17 03:49:01 PST
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:] 2006-02-22 02:46:10 PST
v ff on 1.7.13 on windows/linux/mac, moz on 1.7.13 on windows.

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