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

[Frederic Parain]

> On Aug 9, 2019, at 15:48, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> 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?

Yes, void java_lang_StackFrameInfo::set_bci(oop element, int value)
otherwise value < 65536 is always true.

> 
> It might be even better to get the method from <element>, and assert
> 
>     value >= 0 && value < method->code_size()

The set_bci() method is just a setter to the Java object, I don’t
think its role is to check the consistency of the value.

Fred

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