[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss corresponding Release
From:       David Holmes <david.holmes () oracle ! com>
Date:       2018-12-19 6:20:49
Message-ID: 0e0a64cf-0047-417e-f0f6-351e60e3704e () oracle ! com
[Download RAW message or body]

On 19/12/2018 3:19 am, Baesken, Matthias wrote:
> Hello, here  is an updated  webrev :
> 
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8215411.1/
> 
> sawindbg.cpp  has been adjusted .
> The exception cases mentioned  now also  call   env->ReleaseByteArrayElements  .

Looks good - thanks.

One minor style nit:

725   if(env->ExceptionOccurred()) {

Please add a space after "if" (yes I see the macros have this wrong too).

No need for updated webrev.

Thanks,
David


> Best regards, Matthias
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes@oracle.com>
>> Sent: Dienstag, 18. Dezember 2018 10:04
>> To: Baesken, Matthias <matthias.baesken@sap.com>; 'hotspot-
>> dev@openjdk.java.net' <hotspot-dev@openjdk.java.net>; serviceability-
>> dev@openjdk.java.net; security-dev@openjdk.java.net; JC Beyler
>> <jcbeyler@google.com>
>> Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss
>> corresponding Release
>>
>> 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/function
>> s.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@oracle.com>
>>>> Sent: Dienstag, 18. Dezember 2018 01:20
>>>> To: Baesken, Matthias <matthias.baesken@sap.com>; 'hotspot-
>>>> dev@openjdk.java.net' <hotspot-dev@openjdk.java.net>; serviceability-
>>>> dev@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
>>>>>>
[prev in list] [next in list] [prev in thread] [next in thread]