RFR: 8331731: ubsan: relocInfo.cpp:155:30: runtime error: applying non-zero offset 18446744073709551614 to null pointer [v2]

[Axel Boldt-Christmas]

On Wed, 29 May 2024 10:04:14 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

>> When running on macOS with ubsan enabled, we see some issues in relocInfo  (hpp and cpp); those already occur in the build quite early.
>> 
>> /jdk/src/hotspot/share/code/relocInfo.cpp:155:30: runtime error: applying non-zero offset 18446744073709551614 to null pointer
>> 
>> Similar happens when we add to the _current pointer
>>     _current++;
>> this gives :
>> relocInfo.hpp:606:13: runtime error: applying non-zero offset to non-null pointer 0xfffffffffffffffe produced null pointer
>> 
>> Seems the pointer subtraction/addition worked so far, so it might be an option to disable ubsan for those 2 functions.
>
> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
> 
>   use template functions

This seems to be the same as [JDK-8300821](https://bugs.openjdk.org/browse/JDK-8300821). (Changeset 01312a002ba27bfbfebb9fde484ca34ebde0704c)

The miss here seems to be that `has_loc` does not mean "This CodeSection has relocatations". But means "This CodeSection has allocated a relocations buffer". I believe the correct check would be `cs->locs_count() == 0`

--- a/src/hotspot/share/asm/codeBuffer.cpp
+++ b/src/hotspot/share/asm/codeBuffer.cpp
@@ -525,7 +525,7 @@ void CodeBuffer::finalize_oop_references(const methodHandle& mh) {
   for (int n = (int) SECT_FIRST; n < (int) SECT_LIMIT; n++) {
     // pull code out of each section
     CodeSection* cs = code_section(n);
-    if (cs->is_empty() || !cs->has_locs()) continue;  // skip trivial section
+    if (cs->is_empty() || cs->locs_count() == 0) continue;  // skip trivial section
     RelocIterator iter(cs);
     while (iter.next()) {
       if (iter.type() == relocInfo::metadata_type) {
@@ -793,7 +793,7 @@ void CodeBuffer::relocate_code_to(CodeBuffer* dest) const {
   for (int n = (int) SECT_FIRST; n < (int)SECT_LIMIT; n++) {
     // pull code out of each section
     const CodeSection* cs = code_section(n);
-    if (cs->is_empty() || !cs->has_locs()) continue;  // skip trivial section
+    if (cs->is_empty() || cs->locs_count() == 0) continue;  // skip trivial section
     CodeSection* dest_cs = dest->code_section(n);
     { // Repair the pc relative information in the code after the move
       RelocIterator iter(dest_cs);
@@ -1057,7 +1057,7 @@ void CodeSection::print(const char* name) {
                 name, p2i(start()), p2i(end()), p2i(limit()), size(), capacity());
   tty->print_cr(" %7s.locs = " PTR_FORMAT " : " PTR_FORMAT " : " PTR_FORMAT " (%d of %d) point=%d",
                 name, p2i(locs_start()), p2i(locs_end()), p2i(locs_limit()), locs_size, locs_capacity(), locs_point_off());
-  if (PrintRelocations) {
+  if (PrintRelocations && locs_size != 0) {
     RelocIterator iter(this);
     iter.print();
   }

There is also the following which perhaps should assert that there is a relocation at the call pc. As it makes some assumptions about being able to patch its type. (I am not familiar with this code.) https://github.com/openjdk/jdk/blob/4acafb809c66589fbbfee9c9a4ba7820f848f0e4/src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp#L434 

As for the `RelocIterator::RelocIterator(nmethod* nm, address begin, address limit)` iterator it is less clear to me if it  should be guarded agains nullptr from outside. 

So something would still have to be done about the ubsan.

Much of this seems to be about optimising the `RelocIterator`. But it does not seem worth either the cpu cycles to execute the constructor (nor the cognitive brain cycles to reason about why the constructor is valid when current == -sizeof(relocInfo*)), when we can easily check from the callsite that there are no relocations in our nmethod or code section.

Unsure if solving this by type casting is just hiding underlying design issues.

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

PR Comment: https://git.openjdk.org/jdk/pull/19424#issuecomment-2139709521