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

[Rahul Raghavan]

Thank you Vladimir Kozlov, Vladimir Ivanov, Dean Long for the comments 
and suggestions.

Please review the following points

-- simple fix in InitializeNode::can_capture_store() as suggested by 
Vladimir Ivanov:
   >> "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;
   }"
   > "I don't think we can use is_mismatched_access(), because we seem 
to have the same problem even if int[] is used."

Yes, reconfirmed assert failure for this fix.
#  assert((end_offset % BytesPerInt) == 0) failed: odd end offset



-- New fix in InitializeNode::complete_stores() as suggested by Dean.
https://bugs.openjdk.java.net/browse/JDK-8202414?focusedCommentId=14254276&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14254276

Found following assert failure for this fix proposal, with the new 
Test8202414.java test (similar to the case with earlier initial fix 
proposal from Vladimir Kozlov)
#  assert(!do_zeroing || zeroes_done >= next_init_off) failed: don't 
miss any

Could not find another correct working fix in 
InitializeNode::complete_stores().



-- Latest webrev with last working fix in 
InitializeNode::can_capture_store() with updated Test8202414:

<webrev.02> - http://cr.openjdk.java.net/~rraghavan/8202414/webrev.03/

Could not find any failure issues with this fix.
So can we go ahead with this fix in InitializeNode::can_capture_store() 
itself?


Thanks,
Rahul


On 03/04/19 5:32 AM, Vladimir Ivanov wrote:
> 
>> 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
>>>>>>>
>>>>>>>
>>>>
>>