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

[Vladimir Kozlov]

Then, please, add label "no-regtest" and this comment to the bug report. 
I will push the fix tomorrow.

Thanks,
Vladimir

On 2/5/13 7:00 PM, David Chase wrote:
> 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
>>>
>