Hello, here is an updated webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8215411.1/ sawindbg.cpp has been adjusted . The exception cases mentioned now also call env->ReleaseByteArrayElements . Best regards, Matthias > -----Original Message----- > From: David Holmes <david.hol...@oracle.com> > Sent: Dienstag, 18. Dezember 2018 10:04 > To: Baesken, Matthias <matthias.baes...@sap.com>; 'hotspot- > d...@openjdk.java.net' <hotspot-...@openjdk.java.net>; serviceability- > d...@openjdk.java.net; security-dev@openjdk.java.net; JC Beyler > <jcbey...@google.com> > Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss > corresponding Release > > Hi Matthias, > > On 18/12/2018 6:52 pm, Baesken, Matthias wrote: > > Hi JC / David, thanks for the input . > > > > I'm not really sure what you want me to change David, am I right that I > > can > keep the changes to > > > > src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m > > src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c > > Yes. > > > but adjust > > > > src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp > > > > to also handle theis potential early return > > > > 723 IDebugDataSpaces* ptrIDebugDataSpaces = (IDebugDataSpaces*) > env->GetLongField(obj, > > 724 > > ptrIDebugDataSpaces_ID); > > 725 CHECK_EXCEPTION_(0); > > > > ? > > And I think: > > 730 THROW_NEW_DEBUGGER_EXCEPTION_("Windbg Error: ReadVirtual > failed!", 0); > > as I assume that won't actually return normally. > > >>> Also you change seem wrong to me because it will commit the changes > in > >>> the buffer even if we "abort" here: > >>> > >>> 735 if (bytesRead != numBytes) { > >>> 736 return 0; > >>> 737 } > >>> > > > > The spec says : > > > > > https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function > s.html > > > >> Release<PrimitiveType>ArrayElements Routines > >> void Release<PrimitiveType>ArrayElements(JNIEnv *env, ArrayType > array, NativeType *elems, jint mode); > >> > >> A family of functions that informs the VM that the native code no longer > needs access to elems. The elems argument is a pointer derived from array > using the corresponding > >> Get<PrimitiveType>ArrayElements() function. If necessary, this function > copies back all changes made to elems to the original array. > > > > So shouldn't I tell the VM , that the native code no longer needs > > access to > bytePtr before returning here > > > > 735 if (bytesRead != numBytes) { > > 736 return 0; > > 737 } > > > > ? > > There are two aspects to release: > 1. the actual release (or "I don't need this any more") > 2. committing any changes made back to the original array > > This code does: > > 728 if (ptrIDebugDataSpaces->ReadVirtual((ULONG64) address, (PVOID) > bytePtr, > 729 (ULONG)numBytes, &bytesRead) != > S_OK) { > > which writes into the array. It then checks if we wrote what we expected: > > 735 if (bytesRead != numBytes) { > 736 return 0; > 737 } > > If we didn't read what was expected what should happen to the original > array? Should it be left untouched or updated with the unexpected input? > I would say left untouched and that is what the original code did. > > If everything succeeds you should do the release with mode 0; but if > taking an error exit the release should use mode JNI_ABORT so no changes > are written back. > > Cheers, > David > > > Best regards, Matthias > > > > > >> -----Original Message----- > >> From: David Holmes <david.hol...@oracle.com> > >> Sent: Dienstag, 18. Dezember 2018 01:20 > >> To: Baesken, Matthias <matthias.baes...@sap.com>; 'hotspot- > >> d...@openjdk.java.net' <hotspot-...@openjdk.java.net>; serviceability- > >> d...@openjdk.java.net; security-dev@openjdk.java.net > >> Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss > >> corresponding Release > >> > >> Correction ... > >> > >> On 18/12/2018 8:25 am, David Holmes wrote: > >>> Hi Matthias, > >>> > >>> On 17/12/2018 6:59 pm, Baesken, Matthias wrote: > >>>> Hello, please review the following change. > >>>> I noticed that we miss at some places (for example in case of early > >>>> returns) > >>>> where GetByteArrayElements is used, the corresponding > >>>> ReleaseByteArrayElements call. > >>>> > >>>> In VirtualMachineImpl.c I also removed a check for isCopy (is the > >>>> returned byte array a copy ?) > >>>> because from what I read at > >>>> > >>>> > >> > https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function > >> s.html > >>>> > >>>> > >>>> the ReleaseByteArrayElements routine should always be called. > >>> > >>> That's not clear to me. My reading is that you only need to release if > >>> you have changes you need to ensure are written back and that is only > >>> needed if a copy was made. > >> > >> Jc pointed out to me that if a copy is made you may need to release to > >> avoid a memory leak. But that is where the mode flags come in - and > >> again only relevant if a copy was made. > >> > >> But as per: > >> > >> https://developer.android.com/training/articles/perf-jni > >> > >> if a copy is not made and pinning is used (as with the "critical" > >> variants) then you also need to release to un-pin. > >> > >> So code should call release, with an appropriate mode, based on whether > >> isCopy was true**, and whether changed data should be written back. > >> > >> ** mode is ignored if not working on a copy so you can just set it > >> assuming a copy was made. > >> > >> Note that the hotspot implementation always makes a copy for > >> GextXXXArrayElements, and the ReleaseXXXArrayElements knows this > and > >> so > >> will always free the buffer that is passed in (other than for mode > >> JNI_COMMIT of course). > >> > >> Cheers, > >> David > >> ----- > >> > >>> That said, the change seem semantically correct and harmless in this > case. > >>> --- > >>> > >>> src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp > >>> > >>> AFAICS there are still multiple exit paths via CHECK_EXCEPTION_(0) and > >>> THROW_NEW_DEBUGGER_EXCEPTION_ that won't release the buffer. > >>> > >>> Also you change seem wrong to me because it will commit the changes > in > >>> the buffer even if we "abort" here: > >>> > >>> 735 if (bytesRead != numBytes) { > >>> 736 return 0; > >>> 737 } > >>> > >>> so it seems to me if you really want to release on all paths then the > >>> error paths should use a mode of JNI_ABORT and only the success path > >>> should use mode 0. > >>> > >>> --- > >>> > >>> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m > >>> > >>> This change seems okay, though again potentially not necessary - as we > >>> never commit any changes. > >>> > >>> Thanks, > >>> David > >>> ----- > >>> > >>>> > >>>> bug/webrev : > >>>> > >>>> https://bugs.openjdk.java.net/browse/JDK-8215411 > >>>> > >>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8215411.0/ > >>>> > >>>> > >>>> Thanks, Matthias > >>>>
