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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
12 years ago
6 years ago

People

(Reporter: bc, Assigned: Igor Bukanov)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, regression, verified1.7.13, verified1.8
Points:
---
Bug Flags:
blocking1.8rc1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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()
(Assignee)

Comment 1

12 years ago
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.
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?
(Assignee)

Comment 4

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #199198 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #199268 - Flags: review?(brendan)
(Assignee)

Comment 5

12 years ago
(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+
(Assignee)

Comment 8

12 years ago
Created attachment 199274 [details] [diff] [review]
Patch to commit: Fix 2 + English language nits
Attachment #199268 - Attachment is obsolete: true
(Assignee)

Comment 9

12 years ago
(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

12 years ago
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?
(Reporter)

Comment 11

12 years ago
(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

Updated

12 years ago
Severity: major → critical
Keywords: crash

Comment 13

12 years ago
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
Last Resolved: 12 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?
(Reporter)

Comment 15

12 years ago
trunk looks ok with no regressions I can find so far.
(Reporter)

Comment 16

12 years ago
(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.
(Reporter)

Updated

12 years ago
Flags: testcase+

Comment 17

12 years ago
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

Updated

12 years ago
Attachment #199198 - Flags: approval1.8rc1?

Updated

12 years ago
Attachment #199274 - Flags: approval1.8rc1? → approval1.8rc1+

Comment 19

12 years ago
311497 is now on the branch. Are we ready to land this now? 
Fix checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
(Reporter)

Comment 21

12 years ago
no crash firefox 1.5 rc2 winxp/linux
Keywords: fixed1.8 → verified1.8

Comment 22

12 years ago
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.
Keywords: fixed-aviary1.0.8, fixed1.7.13
(Reporter)

Comment 25

12 years ago
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
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8
(Reporter)

Comment 26

12 years ago
v ff on 1.7.13 on windows/linux/mac, moz on 1.7.13 on windows.
Keywords: fixed1.7.13 → verified1.7.13
Crash Signature: [@ JS_malloc]
You need to log in before you can comment on or make changes to this bug.