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

[Vladimir Ivanov]

Sorry, hit "send" too early. Please, ignore.

Best regards,
Vladimir Ivanov

On 27/03/2019 23:21, Vladimir Ivanov wrote:
> 
> 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

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