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

[Kim Barrett]

On Fri, 2 Dec 2022 10:19:33 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?
>
> Andrew Haley has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Feedback from reviewers

Changes requested by kbarrett (Reviewer).

src/hotspot/share/runtime/vmStructs.hpp line 193:

> 191: #define CHECK_NONSTATIC_VM_STRUCT_ENTRY(typeName, fieldName, type) {               \
> 192:     char space[sizeof (typeName)];                                                 \
> 193:     typeName *dummyObj = (typeName *)space; type* dummy = &dummyObj->fieldName;    \

The member value type checking could be written more explicitly (and IMO more
clearly, though metaprogramming :) ) as:


#define CHECK_NONSTATIC_VM_STRUCT_ENTRY(typeName, fieldName, type) { \
  std::static_assert( \
    std::is_convertible< \
      std::add_pointer_t<decltype(std::declval<typeName>().fieldName)>, \
      std::add_pointer_t<type>>::value, \
    "type mismatch for " XSTR(fieldName) " member of " XSTR(typeName)); \
  assert(offset_of(typeName, fieldName) < sizeof(typeName), "..."); \
}


except that uses `std::declval`, which is in `<utility>`, which is not (yet)
permitted for use in HotSpot code.  Maybe that will change someday.  In the
meantime, we could add our own definition to globalUtilities.hpp, since it is
quite trivial.


template<typename T>
std::add_rvalue_reference_t<T> declval() noexcept;


I tried using `std::is_same` instead of `std::is_convertible`, which seems
like it might be better. However, there were a number of failures because some
uses are not exact matches. There were differences in volatile, in base vs
derived types, and reference vs non-reference types (if you drop the
conversion to pointers).

This dodges the whole question of alignment of the space buffer.

src/hotspot/share/runtime/vmStructs.hpp line 194:

> 192:     char space[sizeof (typeName)];                                                 \
> 193:     typeName *dummyObj = (typeName *)space; type* dummy = &dummyObj->fieldName;    \
> 194:     assert(offset_of(typeName, fieldName) < sizeof(typeName), "Illegal nonstatic struct entry, field offset too large"); \

I think what this assert is really trying to do is verify that fieldName designates an ordinary data member
rather than a static data member.  That's probably possible to determine via some metaprogramming
rather than using `offset_of`.  I'll see if I can come up with something.

src/hotspot/share/runtime/vmStructs.hpp line 202:

> 200:     typedef type dummyvtype; typeName *dummyObj = (typeName *)space;               \
> 201:     volatile dummyvtype* dummy = &dummyObj->fieldName;                             \
> 202:   }

I think a better definition for this macro is

#define CHECK_VOLATILE_NONSTATIC_VM_STRUCT_ENTRY(typeName, fieldName, type) \
  CHECK_NONSTATIC_VM_STRUCT_ENTRY(typeName, fieldName, std::add_volatile_t<type>)

(and `#include <type_traits>`)

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

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