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/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 }
?
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.hol...@oracle.com>
Sent: Dienstag, 18. Dezember 2018 01:20
To: Baesken, Matthias <matthias.baes...@sap.com>; 'hotspot-
d...@openjdk.java.net' <hotspot-...@openjdk.java.net>; serviceability-
d...@openjdk.java.net; security-dev@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
