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

[David Holmes]

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.holmes at oracle.com>
>> Sent: Dienstag, 18. Dezember 2018 01:20
>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>; serviceability-
>> dev at openjdk.java.net; security-dev at 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
>>>>