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

[David Holmes]

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