RFR: 8331854: ubsan: copy.hpp:218:10: runtime error: addition of unsigned offset to 0x7fc2b4024518 overflowed to 0x7fc2b4024510

[Stefan Karlsson]

On Tue, 4 Jun 2024 14:11:42 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

> When building with ubsan, we see a number of overflows at this code location :
> 
> /jdk/src/hotspot/share/utilities/copy.hpp:218:10: runtime error: addition of unsigned offset to 0x7fc2b4024518 overflowed to 0x7fc2b4024510
>     #0 0x10b70896d in Copy::conjoint_words_to_higher(HeapWordImpl* const*, HeapWordImpl**, unsigned long) copy.hpp:218
>     #1 0x10c4f78f1 in Node_Array::insert(unsigned int, Node*) node.cpp:2783
>     #2 0x10b8a1386 in Block::insert_node(Node*, unsigned int) block.hpp:134
>     #3 0x10c556630 in PhaseOutput::fill_buffer(C2_MacroAssembler*, unsigned int*) output.cpp:1792
>     #4 0x10c552f6b in PhaseOutput::Output() output.cpp:367
>     #5 0x10b9ba859 in Compile::Code_Gen() compile.cpp:3035
>     #6 0x10b9b7cb1 in Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*) compile.cpp:896
>     #7 0x10b859912 in C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*) c2compiler.cpp:142
>     #8 0x10b9dd4f1 in CompileBroker::invoke_compiler_on_method(CompileTask*) compileBroker.cpp:2305
>     #9 0x10b9dc345 in CompileBroker::compiler_thread_loop() compileBroker.cpp:1963
>     #10 0x10bfd5ebf in JavaThread::thread_main_inner() javaThread.cpp:760
>     #11 0x10bfd5b62 in JavaThread::run() javaThread.cpp:745
>     #12 0x10c9310d6 in Thread::call_run() thread.cpp:221
>     #13 0x10c53ece4 in thread_native_entry(Thread*) os_bsd.cpp:598

FWIW, I was also thinking that this could be written in another way, but I held of because didn't want to derail yet another ubsan review. :)

The reasons why I would have preferred if this were written another way are:

1) The inserted bail-out is placed just before the assert block. This makes the function have a different structure compared to the other functions that palace the invariant checks first. I prefer to keep code consistent.

2) I'm really not a fan of if statements with returns to the right. It makes it much harder to see the return, IMHO. It's as if we want to hide the return instead of showing it prominently.

Now that people have been given alternatives, I can say that I first considered @dean-long's version. It has a drawback that the from and to names are slightly off given that they point to one beyond the current from and to elements. (With that said, this function already uses `count` as both the count and a loop variable, so I'm not sure that would be worse).

@albertnetymk's version is interesting, but I agree with Dean that it is less obvious. I wonder if this could be written something like this:


while (count-- > 0) {
  to[count] = from[count]
}

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

PR Comment: https://git.openjdk.org/jdk/pull/19541#issuecomment-2149101681