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

[Vladimir Ivanov]

On 26/04/2019 19:30, Vladimir Ivanov wrote:
> 
>>> 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().
>>>
>> Yes, I will add following change also along with any final fix.
>>
>>   intptr_t InitializeNode::can_capture_store(StoreNode* st, 
>> PhaseTransform* phase, bool can_reshape) {
>>     const int FAIL = 0;
>> -  if (st->is_unaligned_access()) {
>> +  if (st->is_unaligned_access() || st->is_mismatched_access()) {
>>       return FAIL;
>>     }
> 
> Looks good.

After thinking more about it, I believe new offset alignment check 
supersedes is_unaligned_access(). And is_mismatched_access() is too 
conservative here: what is_mismatched_access() adds here (in addition to 
existing alignment & size checks) is whether type match between location 
and stored value, but what matters for IN are sizes and offsets only.

Type mismatches (e.g., byte vs boolean, char vs short) may cause 
problems when consequent loads are replaced with values from 
initializing stores, but it should be already handled in 
MemNode::can_see_stored_value() and Load?Node::Ideal().

So, it seems both checks (is_unaligned_access() & 
is_mismatched_access()) can be safely omitted.

Best regards,
Vladimir Ivanov

>>> 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.
>>>
>> (i) At first tried following *wrong* change.
>> (confirmed this is not a fix for the issue)
>>
>>   int InitializeNode::captured_store_insertion_point(intptr_t start,
>>                                                      int size_in_bytes,
>>                                                      PhaseTransform* 
>> phase) {
>>    ...........
>>     for (uint i = InitializeNode::RawStores, limit = req(); ; ) {
>>       if (i >= limit)  return -(int)i; // not found; here is where to 
>> put it
>>
>>       Node*    st     = in(i);
>>       intptr_t st_off = get_store_offset(st, phase);
>> -    if (st_off < 0) {
>> +    if ((size_in_bytes != 0) && (st_off % size_in_bytes) != 0) {
>> +      return FAIL;
>> +    } else if (st_off < 0) {
>>         if (st != zero_memory()) {
>>     ............
>>
>>
>>
>> (ii) When working for correct fix in captured_store_insertion_point(),
>> (was looking to get required captured store / get_store_offset value in)
>> found that following additions in captured_store_insertion_point() 
>> actually fixes reported issue; also could not find any other issue 
>> with testing.
>>
>> ............
>>     if (start >= ti_limit)  return FAIL;
>>
>> +  if ((size_in_bytes != 0) && (start % size_in_bytes) != 0) {
>> +    return FAIL;
>> +  }
>> +
>>     for (uint i = InitializeNode::RawStores, limit = req(); ; ) {
>> ...........
>>
>>
>>
>> (iii) So if above (ii) is a legal/valid fix, then again can the best 
>> location of the fix be as following in can_capture_store() itself?
>>
>> intptr_t InitializeNode::can_capture_store(StoreNode* st, 
>> PhaseTransform* phase, bool can_reshape) {
>>     ...................
>>     AllocateNode* alloc = AllocateNode::Ideal_allocation(adr, phase, 
>> offset);
>>     if (alloc == NULL)
>>       return FAIL;                // inscrutable address
>>     if (alloc != allocation())
>>       return FAIL;                // wrong allocation!  (store needs 
>> to float up)
>> +  int size_in_bytes = st->memory_size();
>> +  if ((size_in_bytes != 0) && (offset % size_in_bytes) != 0) {
>> +    return FAIL;
>> +  }
>>     Node* val = st->in(MemNode::ValueIn);
>>     ...........
>>     return offset;                // success
>>   }
>>
>>
>>
>> (iv) OR  something like following in capture_store() ?
>>   Node* InitializeNode::capture_store(StoreNode* st, intptr_t start,
>>                                       PhaseTransform* phase, bool 
>> can_reshape) {
>>     assert(stores_are_sane(phase), "");
>>
>>     if (start < 0)  return NULL;
>>     assert(can_capture_store(st, phase, can_reshape) == start, "sanity");
>>
>> +  int size_in_bytes = st->memory_size();
>> +  if ((size_in_bytes != 0) && (start % size_in_bytes) != 0) {
>> +    return NULL;
>> +  }
>>     Compile* C = phase->C;
>> -  int size_in_bytes = st->memory_size();
>>     int i = captured_store_insertion_point(start, size_in_bytes, phase);
>>     if (i == 0)  return NULL;     // bail out
>>     ..................
>>
>>
>>
>> (v) Please tell me if I am missing something and none above is valid 
>> fix. Then should work for a correct fix in 
>> captured_store_insertion_point().
> 
> You are right, I missed that IN::captured_store_insertion_point() 
> inspects already other stores which are already captured. Sorry for the 
> confusion.
> 
> I agree that IN::can_capture_store() is the right place to put the fix 
> in and I like (iii). (Just add a comment, "// mismatched access" is enough)
> 
> Best regards,
> Vladimir Ivanov
> 
>>>
>>> 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.
>>>
>> Yes, I will open a new JBS task to address this.
>>
>>
>> Thanks,
>> Rahul
>>
>>> Best regards,
>>> Vladimir Ivanov