348810 - Crash when sorting array with only holes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

My patch for bug 322135 adds code to array_sort from js/src/jsarray.c to shrink the allocated array when it contains holes. But that did not take into account that the array can contain just holes leading to calling realloc(pointer, 0) which  could return NULL. Since the code assumes that NULL from realloc always indicates realloc failure on shrinking, it restore the pointer back to the original value leading to double free later when the function returns.

A trivial example under js shell demonstrates the problem on Linux:

~/m/trunk/mozilla/js/src> js
js> var a= Array(1)
js> a.sort
function sort() {
    [native code]
}
js> a.sort()
typein:3: out of memory
*** glibc detected *** double free or corruption (fasttop): 0x08168b10 ***
Aborted

Comment 1

[Igor Bukanov]

Attachment: Fix (stop playing VM games!)

The patch just removes that broken VM optimization added in the patch for bug 322135. Instead of trying to play realloc games the proper optimization for sorting sparse arrays is enumerate its existing entries instead of scanning the whole [0, length) range. But that is for a different bug. 

Given the amount of badness in that patch I think I should go on vacation earlier. I created the initial version of it a day before departure and instead of just fixing the OEM problem I added few "optimizations" there like that unshift problem and now the issue with sorting.

Comment 2

[Igor Bukanov]

I committed the patch from comment 1 to the trunk.

Comment 3

[Robert Sayre]

Comment on attachment 233982 [details] [diff] [review]
Fix (stop playing VM games!)

blocking feeds

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233982 [details] [diff] [review]
Fix (stop playing VM games!)

a=beltzner on behalf of drivers for the MOZILLA_1_8_BRANCH, please mark fixed1.8.1 when you land.

Comment 5

[Mike Schroepfer]

Do we have a good regression test in the js suite to cover this?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch (sorry Igor, drivers told me to do it!).

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to comment #5)
> Do we have a good regression test in the js suite to cover this?
> 

I will add this test in a bit, but overall: no we do not have good coverage.

Comment 8

[Bob Clary [:bc] (inactive)]

Checking in regress-348810.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-348810.js,v  <--  regress-348810.js
initial revision: 1.1

Comment 9

[Bob Clary [:bc] (inactive)]

verified fixed 1.8, 1.9 20060818 windows/mac*/linux