RFR: [XS] 8215411: some GetByteArrayElements calls miss corresponding Release

[Baesken, Matthias]

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.holmes at oracle.com>
> Sent: Dienstag, 18. Dezember 2018 01:20
> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>; serviceability-
> dev at openjdk.java.net; security-dev at 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
> >>