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
