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

Tue Dec 18 17:19:21 UTC 2018

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.holmes at oracle.com>
> Sent: Dienstag, 18. Dezember 2018 10:04
> 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; JC Beyler
> <jcbeyler at 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.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
> >>>>