RFR: JDK-8294902: Undefined Behavior in C2 regalloc with null references

[Vladimir Ivanov]

On Mon, 31 Oct 2022 14:40:32 GMT, Andrew Haley <aph at openjdk.org> wrote:

> This patch fixes the remaining null pointer dereference bugs that I know of.
> 
> For the main bug, C2 was using a null reference to indicate an uninitialized `Node_List`. I replaced the null reference with a static sentinel.
> 
> I also turned on `-fsanitize=null` and found and fixed a bunch of other null pointer dereferences. With this,I have run a full bootstrap and tier1 tests with `-fsanitize=null` enabled.
> 
> I have checked that the code generated by GCC is not worse in any significant way, so I don't expect to see any performance regressions.
> 
> I'd like to enable `-fsanitize=null` in debug builds to prevent regressions in this area. What do you think?

Minor comments/suggestions. Otherwise, looks good.

src/hotspot/share/oops/instanceKlass.cpp line 390:

> 388:   // Record dependency to keep nest host from being unloaded before this class.
> 389:   ClassLoaderData* this_key = class_loader_data();
> 390:   if (this_key != NULL) {

The code assumes `this_key != NULL`. Do we need an assert/guarantee here?

src/hotspot/share/opto/bytecodeInfo.cpp line 66:

> 64:     assert(!caller_jvms->should_reexecute(), "there should be no reexecute bytecode with inlining");
> 65:   }
> 66:   assert(_caller_jvms == NULL

I'd reshape the code and either get rid of `_caller_jvms` initialization on line 47 or replace it with `_caller_jvms(NULL),`. 

Then, I'd guard `_caller_jvms` initialization by `caller_jvms != NULL` and move the assert under the guard:

  if (caller_jvms != NULL) {
    // Keep a private copy of the caller_jvms:
    _caller_jvms = new (C) JVMState(caller_jvms->method(), caller_tree->caller_jvms());
    _caller_jvms->set_bci(caller_jvms->bci());
    assert(!caller_jvms->should_reexecute(), "there should be no reexecute bytecode with inlining");
    assert(caller_jvms->same_calls_as(_caller_jvms), "consistent JVMS");
  }


Or introduce a helper method which does a shallow copy of `caller_jvms` as part of initializing store on line 47.

src/hotspot/share/opto/node.hpp line 1528:

> 1526: public:
> 1527:   Node_Array(Arena* a, uint max = OptoNodeListSize) : _a(a), _max(max) {
> 1528:     if (a != NULL) {

Add `assert(a != NULL, "...")` here?

-------------

Marked as reviewed by vlivanov (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10920