[13] RFR: 8202414: Unsafe write after primitive array creation may result in array length change

[Vladimir Ivanov]

> I agree that we need better regression tests if we go this route. Do we 
> have enough regression tests for the is_unaligned_access() case to 
> enable that optimization first?

I haven't done any extensive research, but I believe existing tests 
provide poor coverage for initializing stores. The tests I encountered 
under test/hotspot/jtreg/compiler/unsafe/ don't look applicable here.

Best regards,
Vladimir Ivanov

>> Forbidding mismatched accesses in InitializeNode::can_capture_store 
>> (both marked as such and based on actual offset) looks like a safer 
>> fix to me: it keeps InitializeNode::complete_stores() exposed only to 
>> well-behaved accessed.
>>
>> How much do we lose by not capturing mismatched/unaligned initialized 
>> stores? Does it worth optimizing for it?
>>
> 
> It does seem like it would be rare that optimizing it would make a 
> difference, unless we had a microbenchmark that focuses on it.
> 
> dl
> 
>> Best regards,
>> Vladimir Ivanov
>>
>>> On 3/27/19 10:12 AM, Vladimir Ivanov wrote:
>>>> First, I'd like to note that it's a good practice to include problem 
>>>> & root cause descriptions in the request. Otherwise, reviewers have 
>>>> to find that information themselves which complicates review process.
>>>>
>>>> (In this particular case, I found some analysis from the submitter 
>>>> [1] in the bug only after carefully reading through it.)
>>>>
>>>> On 27/03/2019 06:44, Rahul Raghavan wrote:
>>>>> Hi,
>>>>>
>>>>> Thank you Vladimir.
>>>>>
>>>>> Yes, tried following fix.
>>>>> (needed to add checks to avoid SIGFPE crash).
>>>>>
>>>>> +  int size_in_bytes = st->memory_size();
>>>>> +  if ((size_in_bytes != 0) && (get_store_offset(st, phase) % 
>>>>> size_in_bytes) != 0) {
>>>>> +    return FAIL;
>>>>> +  }
>>>>>
>>>>>
>>>>> <webrev.02> - http://cr.openjdk.java.net/~rraghavan/8202414/webrev.02/
>>>>
>>>> It seems the problem is due to mismatched unsafe store being 
>>>> captured as a initializing one. Why not check for it explicitly?
>>>>
>>>>    if (st->is_unaligned_access() || st->is_mismatched_access()) {
>>>>      return FAIL;
>>>>    }
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>>
>>>> [1]
>>>>
>>>> For your convenience, our analysis shows the problem may relate to 
>>>> array InitializeNode logic.
>>>> It `capture_store` the the memory write of Unsafe.putInt.
>>>> Since the putInt occupied offset range [17, 21] from the array pointer,
>>>> then it decided to `clear_memory` of offset range [16, 17] of the 
>>>> array pointer.
>>>> This range actually cannot pass the assert "assert((end_offset % 
>>>> BytesPerInt) == 0, "odd end offset")".
>>>> While in jvm product mode, without the assert, the compiler falsely 
>>>> calculated to clear range [13, 17],
>>>> which will clear the three most significant bytes of the `length` of 
>>>> this array.
>>>>
>>>>
>>>>>
>>>>> Confirmed no issues with testing for this revised fix.
>>>>>
>>>>> Thanks,
>>>>> Rahul
>>>>>
>>>>> On 26/03/19 1:03 AM, Vladimir Kozlov wrote:
>>>>>>
>>>>>> Suggestion:
>>>>>>
>>>>>> if ((get_store_offset(st, phase) % st->memory_size()) != 0) {
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>>
>>>
>