RFR: 8181085: Race condition in method resolution may produce spurious NullPointerException

[Andrew Dinn]

On 25/05/17 14:30, Andrew Haley wrote:
> On 25/05/17 14:12, Andrew Dinn wrote:
>> The following webrev fixes a race condition that is present in jdk10 and
>> also jdk9 and jdk8. It is caused by a misplaced volatile keyword that
>> faild to ensure correct ordering of writes by the compiler. Reviews welcome.
> 
> Can you explain why we don't need a memory fence there?

We do need one and we have one.

The assignments executed in the relevant method in cpCache.cpp (i.e.
ConstantPoolCacheEntry::set_direct_or_vtable_call) are

  . . .
  set_f1(method());
  . . .
  set_bytecode_1(invoke_code);
  . . .

If you look at the definition of these two methods they are

  void set_f1(Metadata* f1) {
    Metadata* existing_f1 = (Metadata*)_f1; // read once
    assert(existing_f1 == NULL || existing_f1 == f1, "illegal field
change");
    _f1 = f1;
  }

and

void ConstantPoolCacheEntry::set_bytecode_1(Bytecodes::Code code) {
#ifdef ASSERT
  // Read once.
  volatile Bytecodes::Code c = bytecode_1();
  assert(c == 0 || c == code || code == 0, "update must be consistent");
#endif
  // Need to flush pending stores here before bytecode is written.
  OrderAccess::release_store_ptr(&_indices, _indices | ((u_char)code <<
bytecode_1_shift));

On x86 the release_store_ptr operation just reduces to an assignment of
volatile field _indices. That alone doesn't stop the compiler
re-ordering it before the assignment of f1. Making both fields volatile
does stop them being re-ordered.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander