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

[Rahul Raghavan]

Thank you Vladimir Ivanov for suggestions.

Request help to review the notes inline below to finalize the fix.

On 25/04/19 12:36 AM, Vladimir Ivanov wrote:
> 
> 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().
> 
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;
    }



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



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