272336 - array_sort uses GC-unsafe temporary memory to hold jsvals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[timeless]

01010113()	
 	js3250.dll!js_Interpret(JSContext * cx=0x02a38600, long * 
result=0x0012ce38)  Line 2800 + 0x213	C
 	js3250.dll!js_Invoke(JSContext * cx=0x00acb7d2, unsigned int 
argc=44271104, unsigned int flags=1232440)  Line 958 + 0xa	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x06e8c0e4, JSObject * 
obj=0x027fc860, long fval=41928824, unsigned int flags=0, unsigned int argc=2, 
long * argv=0x0012cef0, long * rval=0x0012cf08)  Line 1035 + 0xe	C
 	js3250.dll!sort_compare(const void * a=0x072b0c30, const void * 
b=0x072b0c34, void * arg=0x0012cf9c)  Line 769 + 0x55	C
 	js3250.dll!HeapSortHelper(int building=1, HSortArgs * hsa=0x05b4fd98, 
unsigned int lo=109, unsigned int hi=313)  Line 691 + 0x20	C
 	js3250.dll!js_HeapSort(void * vec=0x072b08d0, unsigned int nel=313, 
unsigned int elsize=4, int (const void *, const void *, void *)* 
cmp=0x00ab6074, void * arg=0x0012cf9c)  Line 725 + 0xc	C
>	js3250.dll!array_sort(JSContext * cx=0x02a38600, JSObject * 
obj=0x027fbca0, unsigned int argc=313, long * argv=0x06e8c0dc, long * 
rval=0x0012d010)  Line 889 + 0x30	C
 	js3250.dll!js_Invoke(JSContext * cx=0x00acb7d2, unsigned int 
argc=44271104, unsigned int flags=1232440)  Line 941 + 0x11	C

Comment 1

[Brendan Eich [:brendan]]

What, no IRC log?  Here's the relevant part:

* spidercrasher reads js.exe output for dis(scr_compare)
<brendan> so can you verify that fp->argv[1] is the same value as obj at the
crash site ?
<spidercrasher>  (void*)fp->argv[1] == obj  1 int
<brendan> ok, so how could it be GC'd?
<brendan> it must have been GC'd before fp was set up
<brendan> yikes
<brendan> is array_sort really using unrooted jsval *vec storage?
* spidercrasher looks up
<brendan> file a bug, array_sort uses GC-unsafe temporary memory to hold jsvals
<brendan> nominate for branches
<spidercrasher> sure looks like it
<brendan> thanks
<brendan> see, stacks are helpful ;-)

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

This is painless, good for all active branches.

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 167389 [details] [diff] [review]
proposed fix

Slick!	r=shaver.

Comment 4

[Brendan Eich [:brendan]]

Checked in.  I'll request branch approval on the patch next.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 167389 [details] [diff] [review]
proposed fix

Rare crash fix, no change to web compat w.r.t. Firefox 1.0, so good for 1.7.5.

/be

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 167389 [details] [diff] [review]
proposed fix

a=asa for 1.7.5 checkin

Comment 7

[Asa Dotzler [:asa]]

1.7.5 has shipped. Moving request to 1.7.6.

Comment 8

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Approving, get it in for 1.7.6 quickly. (why not 1.0.1 as well?)

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 167389 [details] [diff] [review]
proposed fix

This hasn't gone into any of the branches it seems, but its too late now. 
Unsetting approval.

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 167389 [details] [diff] [review]
proposed fix

Why not checked into branch yet?
Isn't it considered as a vulnerability?

Comment 11

[timeless]

it's just a crash, you're never going to be able to convince it to go somewhere
useful based on how the gc behaves. that said it really should go into the
branch, it just got lost and now needs new approvals.

Comment 12

[Jay Patel [:jay]]

Comment on attachment 167389 [details] [diff] [review]
proposed fix

Let's get this landed on the branches. a=jay

Comment 13

[Brendan Eich [:brendan]]

Fixed on the branches.

/be