RFR: 8352112: [ubsan] hotspot/share/code/relocInfo.cpp:130:37: runtime error: applying non-zero offset 18446744073709551614 to null pointer [v2]

[Boris Ulasevich]

On Wed, 19 Mar 2025 17:18:50 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> src/hotspot/share/code/codeBlob.cpp line 156:
>> 
>>> 154:   } else {
>>> 155:     // We need unique and valid not null address
>>> 156:     _mutable_data = blob_end();
>> 
>> It makes me a little nervous pointing this value to real data.  When RelocIterator computes `_current = nm->relocation_begin() - 1`, it should never read or write from that address, but how can we guarantee that?  Any non-null address that is guarateed unmapped would do, or a special protetected page like `bad_page` here: https://github.com/openjdk/jdk/blob/8e530633a9d99d7ce585cafd5573cb89212feee7/src/hotspot/share/runtime/safepointMechanism.cpp#L66.  If using protected memory seems like overkill, then I suggest using a static.  Something like this:
>> 
>>    static union {
>>     relocInfo _dummy[1];
>>   } _empty[2];
>>   [...]
>>   _mutable_data = _empty+1;
>> 
>> However, I think this is not the first time we have run into this issue with RelocIterator.  Maybe it's time that we rewrote it to avoid this situation?
>
> How about this?:
> 
> +++ b/src/hotspot/share/code/relocInfo.cpp
> @@ -117,6 +117,8 @@ void relocInfo::change_reloc_info_for_address(RelocIterator *itr, address pc, re
>  // Implementation of RelocIterator
>  
> +static relocInfo dummy_reloc[2];
> +
>  void RelocIterator::initialize(nmethod* nm, address begin, address limit) {
>    initialize_misc();
>  
> @@ -127,8 +129,14 @@ void RelocIterator::initialize(nmethod* nm, address begin, address limit) {
>    guarantee(nm != nullptr, "must be able to deduce nmethod from other arguments");
>  
>    _code    = nm;
> -  _current = nm->relocation_begin() - 1;
> -  _end     = nm->relocation_end();
> +  // Check for no relocations case and use dummy data to avoid referencing wrong data.
> +  if (nm->relocation_size() == 0) {
> +    _current = dummy_reloc;
> +    _end     = dummy_reloc + 1;
> +  } else {
> +    _current = nm->relocation_begin() - 1;
> +    _end     = nm->relocation_end();
> +  }
>    _addr    = nm->content_begin();
>  
>    // Initialize code sections.
> 
> 
> I filed RFE: [JDK-8352426](https://bugs.openjdk.org/browse/JDK-8352426)

We can just add nullptr checks before pointer arithmetic in relocInfo:

diff --git a/src/hotspot/share/code/relocInfo.cpp b/src/hotspot/share/code/relocInfo.cpp
index 7aae32759dd..c694f21e5ca 100644
--- a/src/hotspot/share/code/relocInfo.cpp
+++ b/src/hotspot/share/code/relocInfo.cpp
@@ -127,7 +127,8 @@ void RelocIterator::initialize(nmethod* nm, address begin, address limit) {
   guarantee(nm != nullptr, "must be able to deduce nmethod from other arguments");

   _code    = nm;
-  _current = nm->relocation_begin() - 1;
+  _current = nm->relocation_begin();
+  if (_current != nullptr) { _current--; }
   _end     = nm->relocation_end();
   _addr    = nm->content_begin();

diff --git a/src/hotspot/share/code/relocInfo.hpp b/src/hotspot/share/code/relocInfo.hpp
index 25cca49e50b..b440e713493 100644
--- a/src/hotspot/share/code/relocInfo.hpp
+++ b/src/hotspot/share/code/relocInfo.hpp
@@ -603,7 +603,7 @@ class RelocIterator : public StackObj {

   // get next reloc info, return !eos
   bool next() {
-    _current++;
+    if (_current != nullptr) { _current++; }
     assert(_current <= _end, "must not overrun relocInfo");
     if (_current == _end) {
       set_has_current(false);

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24102#discussion_r2005301702