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

[Vladimir Ivanov]

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

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