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

[David Holmes]

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/functions.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.

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
>