Request for reviews (XS): 8006807: C2 crash due to out of bounds array access in Parse::do_multianewarray

[David Chase]

Vladimir, it's not an easy bug to reproduce.  From below:

"I also couldn't reproduce the problem with a test program which ran
for several hours and constantly compiled a method which allocated
several two- and three-dimensional arrays so I refrained from writing
a regression test for this problem."

On 2013-02-05, at 8:14 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

> Changes are good. But we need a regression test (in test/compiler/) for this. Volker, do you have one? If not, David, could you prepare one?
> 
> David, you don't need to repeat testing you already did. Just run new test through JPRT.
> 
> Thanks,
> Vladimir
> 
> On 2/5/13 8:49 AM, David Chase wrote:
>> This is Volker Simonis's fix to his bug, with added copyright change at
>> the top.
>> 
>> http://cr.openjdk.java.net/~drchase/8006807/webrev.01
>> 
>> [ Volker, in previous email when I said it had been "reviewed", I should
>> have specified that this was not in the formal sense, since I am not an
>> openjdk reviewer, and am still working on commit privilege.  But I did
>> check the patch and the surrounding code with a skeptical eye.]
>> 
>> Quick description of problem: when allocating certain multidimensional
>> arrays the code would index off the end of the bounds array.
>> Normally this is harmless because the bogus data was ignored anyway, but
>> in rare cases the bounds array would be allocated right at the end of a
>> mapping boundary and a crash would result.  Volker's detailed
>> description is included below.
>> 
>> Fix: be aware of the array dimensions when fetching them, and substitute
>> null for the unneeded entries when passing to make_runtime_call.
>> 
>> Testing:
>>   JPRT, successfully.
>>   Jtreg of compiler, passed except for 4 lambda-related asserts that I
>> think are from compiler/library version skew.
>>   Also eyeballed the surrounding source code very carefully, since this
>> is a rare bug that will not reproduce on command.
>> 
>> David
>> 
>> Begin forwarded message:
>> 
>>> Please review the fix for the following issue and submit the change if
>>> you find it appropriate:
>>> 
>>> http://cr.openjdk.java.net/~simonis/webrevs/8006807/
>>> <http://cr.openjdk.java.net/%7Esimonis/webrevs/8006807/>
>>> 
>>> Thechange<http://cr.openjdk.java.net/%7Eiveresov/7058510/webrev.03/>for "7058510:multinewarray
>>> with 6 dimensions uncommon traps in server compiler
>>> <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7058510>" replaced
>>> the fixed sized, stack allocated array '|length|' of length 5 with a
>>> dynamically, arena allocated array of size '|ndimensions + 1|':
>>>   Node** length = NEW_RESOURCE_ARRAY(Node*, ndimensions + 1);
>>>   length[ndimensions] = NULL;  // terminating null for make_runtime_call
>>> 
>>> In cases where '|ndimensions|' is smaller than 4 (and bigger than 1
>>> because the 1-dimensional case is handled specially) this leads to
>>> field accesses beyond the bounds of the '|length|' array later on
>>> during the call to '|make_runtime_call(..)|' which can eventually
>>> crash the compiler:
>>> 
>>>     c = make_runtime_call(RC_NO_LEAF | RC_NO_IO,
>>>                           OptoRuntime::multianewarray_Type(ndimensions),
>>>                           fun, NULL, TypeRawPtr::BOTTOM,
>>>                           makecon(TypeKlassPtr::make(array_klass)),
>>>                           length[0], length[1], length[2],
>>>                           length[3], length[4]);
>>> 
>>> Notice that these crashes are extremely rare, because usually, there
>>> is still some accessible memory beyond the arena allocated '|length|'
>>> array. We only observed four crashes because of this problem in
>>> regular nightly runs of JVM2008 during the last six month. The crashes
>>> only happened on Windows although the problem is platform independent.
>>> I also couldn't reproduce the problem with a test program which ran
>>> for several hours and constantly compiled a method which allocated
>>> several two- and three-dimensional arrays so I refrained from writing
>>> a regression test for this problem.
>>> 
>>> The fix is trivial: just check the value of '|ndimensions|' before
>>> accessing the corresponding '|length|' slots and use|NULL|values for
>>> the cases where the array slots do not exist. Notice that we only
>>> call|make_runtime_call|if '|ndimensions|' is bigger than 1 in which
>>> case '|length|' already contains three entries (see allocation of the
>>> '|length|' array above).
>>> 
>>>     c = make_runtime_call(RC_NO_LEAF | RC_NO_IO,
>>>                           OptoRuntime::multianewarray_Type(ndimensions),
>>>                           fun, NULL, TypeRawPtr::BOTTOM,
>>>                           makecon(TypeKlassPtr::make(array_klass)),
>>>                           length[0], length[1], length[2],
>>>                           (ndimensions > 2) ? length[3] : NULL,
>>>                           (ndimensions > 3) ? length[4] : NULL);
>>> 
>>> 
>>> 
>>> Thank you and best regards,
>>> Volker
>>