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