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
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);
?
> > 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/functions.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 }
?
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
> >>
