[10] Review Request: 8187442 Xcheck:jni produces various "WARNING in native method" in launcher
David Holmes
david.holmes at oracle.com
Tue Sep 12 23:03:02 UTC 2017
On 13/09/2017 7:58 AM, Sergey Bylokhov wrote:
> On 9/12/17 14:28, David Holmes wrote:
>>> But in case of CallStaticObjectMethod() we cannot check for NULL
>>> which can be a valid result, so I added a check for exception and
>>> return 0.
>>
>> My point is that the check is completely redundant. If an exception
>> occurred then the return value is already 0/NULL.
>
> According to the spec of jni there is no information that
> CallStaticObjectMethod() return null/0 when exception is occurred.
> So checkjni is not smart enough or takes this into account.
Sorry - you are right. I keep thinking we have closed these holes in JNI
but we haven't. Even worse the actual implementation will return
uninitialized values for the CallXXXMethod functions!
If we look at:
1543 jstring str = NewPlatformString(env, *strv++);
1544 NULL_CHECK0(str);
1545 (*env)->SetObjectArrayElement(env, ary, i, str);
what currently happens is that if NewPlatformString has an exception
pending due to the CallStaticObjectMethod call then we can get back a
possibly non-zero "random" value. When non-zero the NULL_CHECK passes
and we proceed with the SetObjectArrayElement with a pending exception
and so get the warning. And what you are doing is strengthening the
internal spec of NewPlatformString so that it is ensures 0 is returned
if an exception is pending and hence we skip the SetObjectArrayElement
call. That is fine, but still leaves the problem that you are skipping
the DeleteLocalRef call.
Thanks - and sorry again for my confusion on this.
David
-----
>> In addition this does nothing to clear the pending exception so I can
>> not see how it would affect any warnings.
>
> It does not clear an exception but preserve it instead, and does not use
> the result of the method which produced an exception.
>
>>
>> David
>>
>>> This value will be propagated to JavaMain() and I as far as
>>> understand will stop the execution.
>>>
>>>
>>> On 9/12/17 13:56, David Holmes wrote:
>>>> Hi Sergey,
>>>>
>>>> On 13/09/2017 5:18 AM, Sergey Bylokhov wrote:
>>>>> Hello,
>>>>> Please review the fix for jdk10/jdk10.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8187442
>>>>> Webrev can be found at:
>>>>> http://cr.openjdk.java.net/~serb/8187442/webrev.00
>>>>
>>>> This doesn't look right to me:
>>>>
>>>> str = (*env)->CallStaticObjectMethod(env, cls,
>>>> makePlatformStringMID, USE_STDERR, ary);
>>>> + CHECK_EXCEPTION_RETURN_VALUE(0);
>>>> (*env)->DeleteLocalRef(env, ary);
>>>> return str;
>>>>
>>>> If there is an exception pending you fail to delete the local ref.
>>>> And there's no need to clear the exception before calling
>>>> DeleteLocalRef as that is okay to call with a pending exception. But
>>>> there is no point returning zero if an exception occurred because in
>>>> that case str will already be 0/NULL.
>>>>
>>>> The same here:
>>>>
>>>> 1596 appClass = (*env)->CallStaticObjectMethod(env, cls, mid);
>>>> 1597 CHECK_EXCEPTION_RETURN_VALUE(0);
>>>> 1598 return appClass;
>>>>
>>>> If an exception is pending then appClass will be 0/NULL.
>>>>
>>>> In addition CHECK_EXCEPTION_RETURN_VALUE doesn't clear the pending
>>>> exception so I can't see what warnings this would be clearing up ???
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> The simple application with empty main method produce some
>>>>> "warnings" when Xcheck:jni is used. Because of that it is hard to
>>>>> cleanup other parts of jdk from such warnings.
>>>>>
>>>>> Two additional checks were added, in both cases the new code will
>>>>> return 0 in the same way as NULL_CHECK0 in the same methods.
>>>>>
>>>>>
>>>
>>>
>
>
More information about the core-libs-dev
mailing list