RFR: 6726901: JDWP: ReferenceType.GetValues crashes jvm in case non-static fields are passed

[Chris Plummer]

On Wed, 20 Nov 2024 18:09:08 GMT, Adam Bruce <duke at openjdk.org> wrote:

> This PR fixes a long-standing bug in JDWP where the access flags of a field are not checked before attempting to read it's value. 
> 
> Prior to this change, attempting to read a non-static field would cause a JVM crash, this change corrects that behaviour by returning `INVALID_FIELDID` instead.
> 
> This is my first PR to OpenJDK, so please let me know if I've made any mistakes in the process.
> 
> Cheers,
> Adam

Hi Adam. Thank you for your contribution.

I think the fix should be moved to sharedGetFieldValues(), and should verify that the isStatic flag matches the STATIC modifier of the field. Right now you are only catching ReferenceType.GetValues of a non-static field. You also need to catch ObjectReference.GetValues of a static field. The other option is to add a similar fix to writeFieldValue().

Note that the JDI ObjectReference.getValues() allows access to both static and instance fields, but JDWP ObjectReference.GetValues only allows access to instance fields, not static fields. This is all handled in the JDI implementation of ObjectReferenceImpl.getValues(), which separately uses both ReferenceType.GetValue for static fields and ObjectReference.GetValues for instance fields.

I think JDI might need some fixes to the exception handling. Currently in ReferenceTypeImpl.getValues(), any JDWP exception goes through toJDIException() to convert it to a JDI exception, and it does not handle INVALID_FRAMEID. You'll need to special case it to throw IllegalArgumentException. Same is true in ObjectReferenceImpl.getValues().

You'll need to add tests cases as part of this PR.

Please make sure all of the following tests pass:
   test/jdk/com/sun/jdi
   test/hotspot/jtreg/vmTestbase/nsk/jdi
   test/hotspot/jtreg/vmTestbase/nsk/jdwp
   test/hotspot/jtreg/vmTestbase/nsk/jdb

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

PR Comment: https://git.openjdk.org/jdk/pull/22280#issuecomment-2489486881