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

[dean.long at oracle.com]

That seems like the safest fix for now.

dl

On 4/24/19 5:22 AM, Rahul Raghavan wrote:
> 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
>>>>>>>>
>>>>>>>>
>>>>>
>>>