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

[Vladimir Kozlov]

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
>