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

[Ioi Lam]

On 8/9/19 11:24 AM, Frederic Parain wrote:
> 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 think bci is not really a short. It should be a ushort, since the max 
value is 0xffff. Anyway, I agree that it's best to keep the existing 
function signature, or else we will be adding a lot of type casts 
everywhere.

> 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);
> }
Did you mean to declare <value> as an int in the code above?

It might be even better to get the method from <element>, and assert

     value >= 0 && value < method->code_size()


Thanks
- Ioi

>
> 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
>>