On Wed, 20 Nov 2024 18:09:08 GMT, Adam Bruce <d...@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
