[14] RFR (S): 8231430: C2: Memory stomp in max_array_length() for T_ILLEGAL type

[Vladimir Kozlov]

There is assert in arrayOopDesc::max_array_length() which checks '< T_CONFLICT'.
Next assert 'type2aelembytes(type) != 0' will be triggered for T_VOID.
The assert in type2aelembytes() will be triggered for T_ADDRESS since allow_address argument is false by default.

Which leaves T_METADATA and T_NARROWKLASS to check for since they can't be elements of array. Right?

May be we should have permanent guarantee() in TypeAryPtr::max_array_length() for all types which we don't expect to see 
and not temporary assert().

Thanks,
Vladimir

On 11/27/19 5:54 AM, Vladimir Ivanov wrote:
> http://cr.openjdk.java.net/~vlivanov/8231430/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8231430
> 
> There's a memory stomp happening in max_array_length() for T_ILLEGAL type. T_ILLEGAL type arises as an element basic 
> type for a merge of 2 primitive arrays (bottom[]). max_array_length() does some input normalization (T_ILLEGAL => 
> T_BYTE), but first it acquires a reference to the a cache slot which is out-of-bounds (T_ILLEGAL = 99 vs T_CONFLICT = 19).
> 
> I was able to reproduce the problem as a corruption of one of the OOPs in Universe::_mirrors array which happened to be 
> put close enough to max_array_length_cache in memory.
> 
> I propose to completely remove the cache. arrayOopDesc::max_array_length() doesn't look too expensive and the method is 
> not used on a hot path anywhere.
> 
> Also, I put an assert for T_VOID, T_CONFLICT, T_NARROWKLASS cases, but left the logic there (=> T_BYTE) to get more 
> testing before removing them.
> 
> Testing: hs-precheckin-comp, tier1-5.
> 
> Best regards,
> Vladimir Ivanov