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

[Vladimir Ivanov]

On 27/03/2019 17:27, dean.long at oracle.com wrote:
> I don't think we can use is_mismatched_access(), because we seem to have 
> the same problem even if int[] is used.  I took another look at this, 
> and I believe we can fix this in InitializeNode::complete_stores(), 
> while still allowing the captured store optimization.

Yes, good point.

So, what you are saying is that is_mismatched_access() is not sufficient 
to cover all the cases and the only missing case is , right?

I believe checking that offset is in doubn

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
>>>>
>>>>
>