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

[Vladimir Ivanov]

> 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, IMO the main question is what's the best place to fix it:
   InitializeNode::complete_stores() vs InitializeNode::can_capture_store

Capturing stores and then InitializeNode::complete_stores() looks 
attractive, but there are other cases arising with unsafe accesses which 
aren't tested well yet. So, if we choose that route, the regression test 
should be improved. On the other hand, "!st->is_unaligned_access()" 
constraint in InitializeNode::can_capture_store() can be removed.

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?

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