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

[Vladimir Ivanov]

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

A couple of suggestions:

src/hotspot/share/opto/memnode.cpp:

+  int size_in_bytes = st->memory_size();
+  if ((size_in_bytes != 0) && (get_store_offset(st, phase) % 
size_in_bytes) != 0) {
+    return FAIL;
+  }

I'd just add "if (st->is_mismatched_access()) { return FAIL; }" in 
IN::can_capture_store and move the rest into 
IN::captured_store_insertion_point().

It looks like InitializeNode::captured_store_insertion_point() is a 
better place to inspect get_store_offset() and fail on mismatch. It also 
handles negative values returned by get_store_offset() and has a 
consistent handling of "size_in_bytes == 0" case.


PS: Frankly speaking, I'm not comfortable with current handling of 
"size_in_bytes == 0" case:

// If size_in_bytes is zero, do not bother with overlap checks.
int InitializeNode::captured_store_insertion_point(intptr_t start,
                                                    int size_in_bytes,
                                                    PhaseTransform* phase) {

"size_in_bytes == 0" is the case for StoreVectorNodes. I believe the 
logic is there to accommodate bulk initialization with vector stores 
(haven't found the proofs in the code though), but it looks fragile 
(especially when vector operations become more common). It would be nice 
to enable overlap checks for vector operations.

But I'm perfectly fine with handling that separately.

Best regards,
Vladimir Ivanov

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