[9] RFR: 8170465, 8170466: JNI exception pending in jni_util.c:190

[Naoto Sato]

Thanks for the review, Christoph.

I too agree that the null check can be removed, but decided to keep it 
as a precaution as David mentioned.

Naoto

On 12/6/16 11:49 PM, Langer, Christoph wrote:
> Hi Naoto,
>
> the new webrev looks ok to me, too. Thanks for fixing.
>
> And I agree to the point that the NULL check can be removed...if you like.
>
> Best regards
> Christoph
>
>> -----Original Message-----
>> From: core-libs-dev [mailto:core-libs-dev-bounces at openjdk.java.net] On Behalf
>> Of David Holmes
>> Sent: Mittwoch, 7. Dezember 2016 03:15
>> To: Naoto Sato <naoto.sato at oracle.com>; core-libs-dev <core-libs-
>> dev at openjdk.java.net>
>> Subject: Re: [9] RFR: 8170465, 8170466: JNI exception pending in jni_util.c:190
>>
>> On 7/12/2016 11:04 AM, Naoto Sato wrote:
>>> Thanks, David. The fix is modified accordingly:
>>>
>>> http://cr.openjdk.java.net/~naoto/8170465.8170466/webrev.01/
>>
>> Thanks for fixing that.
>>
>> The null checks after the exception checks are now redundant as you can
>> only get a null return from those methods if an exception becomes
>> pending. But leaving them in as a precaution is okay I guess.
>>
>> Thanks,
>> David
>>
>>> Naoto
>>>
>>> On 12/6/16 4:53 PM, David Holmes wrote:
>>>> Hi Naoto,
>>>>
>>>> On 7/12/2016 10:02 AM, Naoto Sato wrote:
>>>>> Hello,
>>>>>
>>>>> Please review the proposed fix to the following issues:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8170465
>>>>> https://bugs.openjdk.java.net/browse/JDK-8170466
>>>>>
>>>>> The proposed fix is located at:
>>>>>
>>>>> http://cr.openjdk.java.net/~naoto/8170465.8170466/webrev.00/
>>>>>
>>>>> These two JNI calls (NewStringUTF() and JNU_CallMethodByName()) in
>>>>> src/java.base/share/native/libjava/jni_util.c can throw an exception.
>>>>> Those possible exceptions need to be properly checked.
>>>>
>>>> JNU_CHECK_EXCEPTION will do a return from the method in which it is
>>>> placed, so you need to be careful to ensure you do all needed cleanup
>>>> before that eg:
>>>>
>>>>  203                 JNU_CHECK_EXCEPTION(env);
>>>>  204                 free(str1);
>>>>
>>>> needs to be reversed else we won't free str1. Similarly:
>>>>
>>>>  210                     JNU_CHECK_EXCEPTION(env);
>>>>  211                     (*env)->DeleteLocalRef(env, s2);
>>>>
>>>> needs to be reversed so we don't leak the local ref. (It is safe to call
>>>> DeleteLocalRef with an exception pending.)
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Naoto