Closed Bug 312069 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: bc, Assigned: igor)

References

()

Details

(4 keywords)

Crash Data

Attachments

(1 file, 2 obsolete files)

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()
Attached patch Fix (obsolete) — Splinter Review
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.
Attachment #199198 - Flags: review?(brendan)
Must-fix for 1.8.

/be
Assignee: general → igor.bukanov
Flags: blocking1.8rc1+
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
Attachment #199198 - Flags: review?(brendan)
Attachment #199198 - Flags: review+
Attachment #199198 - Flags: approval1.8rc1?
Attached patch Fix 2 (obsolete) — Splinter Review
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.
Attachment #199198 - Attachment is obsolete: true
Attachment #199268 - Flags: review?(brendan)
(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 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.
Attachment #199268 - Flags: review+
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
Attachment #199268 - Flags: review?(brendan) → review+
Attachment #199268 - Attachment is obsolete: true
(In reply to comment #7)
> if you land this on the trunk now, it will
> help us to get it onto the branch.

Done

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?
(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. 
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
Severity: major → critical
Keywords: crash
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. 
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

Queueing up.

/be
Attachment #199274 - Flags: superreview+
Attachment #199274 - Flags: review+
Attachment #199274 - Flags: approval1.8rc1?
trunk looks ok with no regressions I can find so far.
(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.
Flags: testcase+
Bob, what more testing needs to happen (if any) before we approve this for the
branch?
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
Depends on: 311497
Attachment #199198 - Flags: approval1.8rc1?
Attachment #199274 - Flags: approval1.8rc1? → approval1.8rc1+
311497 is now on the branch. Are we ready to land this now? 
Fix checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
no crash firefox 1.5 rc2 winxp/linux
Keywords: fixed1.8verified1.8
Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

Bkap needs approval to land for 1.7 trees
Attachment #199274 - Flags: approval1.7.13?
Attachment #199274 - Flags: approval-aviary1.0.8?
Comment on attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits

a=dveditz
Attachment #199274 - Flags: approval1.7.13?
Attachment #199274 - Flags: approval1.7.13+
Attachment #199274 - Flags: approval-aviary1.0.8?
Attachment #199274 - Flags: approval-aviary1.0.8+
This was checked into the 1.7 branch along with my checkin for bug 311497.
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.
Status: RESOLVED → VERIFIED
v ff on 1.7.13 on windows/linux/mac, moz on 1.7.13 on windows.
Crash Signature: [@ JS_malloc]
You need to log in before you can comment on or make changes to this bug.