RFR(XS): JDK-8229375 Memory corruption in the implementation of the stack walk API

[Frederic Parain]

Changing the prototype to set_bci(oop, short) would be nice, but
there’s a lot of code in the JVM still using an int to store the bci.
Just to list a few:

  - java_lang_StackFrameInfo::set_method_and_bci()
  - class BaseFrameStream and its sub-classes JavaFrameStream and LiveFrameStream
  - class javaVFrame and its sub-classes interpretedVFrame and compiledVFrame
  - class jvmtiDeferredLocalVariableSet
  - class BaseBytecodeStream
  - struct BacktraceElement
  - class BasicBlock
  - class CallJavaNode
  - class ciReturnAddress

And at least the first three of this shortened list are used by the stack walk API.

I’d prefer the assert solution based on the JVMS definition of method’s length:

code_length
The value of the code_length item gives the number of bytes in the code array for this method.
The value of code_length must be greater than zero (as the code array must not be empty) and less than 65536.

Which would produce something like this:

void java_lang_StackFrameInfo::set_bci(oop element, short value) {
  assert(value >= 0 && value < 65536, “bci outside of valid range”);
  element->short_field_put(_bci_offset, value);
}

What do yo think?

Fred


> On Aug 9, 2019, at 13:08, Aleksey Shipilev <shade at redhat.com> wrote:
> 
> On 8/9/19 6:54 PM, Frederic Parain wrote:
>> CR: https://bugs.openjdk.java.net/browse/JDK-8229375
>> Webrev: http://cr.openjdk.java.net/~fparain/8229375/webrev.00/index.html
> 
> Oh wow. It apparently foobars the injected "version" field, see the comment in the issue.
> 
> The fix looks okay. Should we least assert the range of the int value we are storing there? Or even
> make set_bci(oop, *short*)?
> 
> -- 
> Thanks,
> -Aleksey
>