[8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

Calvin Cheung
Please review this small fix for adding the check for JNI pending
exception in URLClassPath.c.

This fix is only for jdk8u and isn't applicable to 9.

JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
       (unfortunately it is a closed bug)

webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/

Tests:
     JPRT with -testset hotspot
     RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick

thanks,
Calvin
Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

Jiangli Zhou
Hi Calvin,

I see getUTF() throws OutOfMemoryError if malloc fails before returning NULL. Do you know if there is any case that getUTF() returns NULL without throwing any exception? The usage of getUTF() and exception handling don’t seem to be consistent in existing idk code. Some places check the NULL return value without throwing a new exception, while the other places throws a new OOM without checking for pending exception (like the code you are fixing). Maybe the callee does not need to re-throw the exception for the NULL return value case, and you can add a comment saying “an OutOfMemoryError” has already been thrown by getUTF()?

Thanks,
Jiangli


> On Nov 30, 2015, at 5:13 PM, Calvin Cheung <[hidden email]> wrote:
>
> Please review this small fix for adding the check for JNI pending exception in URLClassPath.c.
>
> This fix is only for jdk8u and isn't applicable to 9.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
>      (unfortunately it is a closed bug)
>
> webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/
>
> Tests:
>    JPRT with -testset hotspot
>    RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick
>
> thanks,
> Calvin

Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

David Holmes
On 1/12/2015 12:10 PM, Jiangli Zhou wrote:
> Hi Calvin,
>
> I see getUTF() throws OutOfMemoryError if malloc fails before returning NULL. Do you know if there is any case that getUTF() returns NULL without throwing any exception? The usage of getUTF() and exception handling don’t seem to be consistent in existing idk code. Some places check the NULL return value without throwing a new exception, while the other places throws a new OOM without checking for pending exception (like the code you are fixing). Maybe the callee does not need to re-throw the exception for the NULL return value case, and you can add a comment saying “an OutOfMemoryError” has already been thrown by getUTF()?

According to classloader.c:

/* Convert java string to UTF char*. Use local buffer if possible,
    otherwise malloc new memory. Returns null IFF malloc failed. */

So NULL implies OOME has been thrown. I'd suggest removing the new code
and the JNU_ThrowOutOfMemory with a comment as Jiangli suggested.

Also note we have the same kinds of patterns in classLoader.c in 8 and 9.

Thanks,
David

> Thanks,
> Jiangli
>
>
>> On Nov 30, 2015, at 5:13 PM, Calvin Cheung <[hidden email]> wrote:
>>
>> Please review this small fix for adding the check for JNI pending exception in URLClassPath.c.
>>
>> This fix is only for jdk8u and isn't applicable to 9.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
>>       (unfortunately it is a closed bug)
>>
>> webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/
>>
>> Tests:
>>     JPRT with -testset hotspot
>>     RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick
>>
>> thanks,
>> Calvin
>
Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

Ioi Lam


On 11/30/15 8:21 PM, David Holmes wrote:

> On 1/12/2015 12:10 PM, Jiangli Zhou wrote:
>> Hi Calvin,
>>
>> I see getUTF() throws OutOfMemoryError if malloc fails before
>> returning NULL. Do you know if there is any case that getUTF()
>> returns NULL without throwing any exception? The usage of getUTF()
>> and exception handling don’t seem to be consistent in existing idk
>> code. Some places check the NULL return value without throwing a new
>> exception, while the other places throws a new OOM without checking
>> for pending exception (like the code you are fixing). Maybe the
>> callee does not need to re-throw the exception for the NULL return
>> value case, and you can add a comment saying “an OutOfMemoryError”
>> has already been thrown by getUTF()?
>
> According to classloader.c:
>
> /* Convert java string to UTF char*. Use local buffer if possible,
>    otherwise malloc new memory. Returns null IFF malloc failed. */
>
> So NULL implies OOME has been thrown. I'd suggest removing the new
> code and the JNU_ThrowOutOfMemory with a comment as Jiangli suggested.
>
> Also note we have the same kinds of patterns in classLoader.c in 8 and 9.
>
I introduced this bug in URLClassPath.c because I copied the code from
ClassLoader.c, which had the same pattern, and has been reported in
https://bugs.openjdk.java.net/browse/JDK-6952230 (5 years!)

- Ioi

> Thanks,
> David
>
>> Thanks,
>> Jiangli
>>
>>
>>> On Nov 30, 2015, at 5:13 PM, Calvin Cheung
>>> <[hidden email]> wrote:
>>>
>>> Please review this small fix for adding the check for JNI pending
>>> exception in URLClassPath.c.
>>>
>>> This fix is only for jdk8u and isn't applicable to 9.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
>>>       (unfortunately it is a closed bug)
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/
>>>
>>> Tests:
>>>     JPRT with -testset hotspot
>>>     RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick
>>>
>>> thanks,
>>> Calvin
>>

Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

Calvin Cheung
Hi Jiangli, David, Ioi,

Thank you for the review.

Here's an updated webrev:
http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.01/

Basically, the JNU_ThrowOutOfMemoryError call has been replaced with a
comment.

thanks,
Calvin

On 11/30/15, 8:52 PM, Ioi Lam wrote:

>
>
> On 11/30/15 8:21 PM, David Holmes wrote:
>> On 1/12/2015 12:10 PM, Jiangli Zhou wrote:
>>> Hi Calvin,
>>>
>>> I see getUTF() throws OutOfMemoryError if malloc fails before
>>> returning NULL. Do you know if there is any case that getUTF()
>>> returns NULL without throwing any exception? The usage of getUTF()
>>> and exception handling don’t seem to be consistent in existing idk
>>> code. Some places check the NULL return value without throwing a new
>>> exception, while the other places throws a new OOM without checking
>>> for pending exception (like the code you are fixing). Maybe the
>>> callee does not need to re-throw the exception for the NULL return
>>> value case, and you can add a comment saying “an OutOfMemoryError”
>>> has already been thrown by getUTF()?
>>
>> According to classloader.c:
>>
>> /* Convert java string to UTF char*. Use local buffer if possible,
>>    otherwise malloc new memory. Returns null IFF malloc failed. */
>>
>> So NULL implies OOME has been thrown. I'd suggest removing the new
>> code and the JNU_ThrowOutOfMemory with a comment as Jiangli suggested.
>>
>> Also note we have the same kinds of patterns in classLoader.c in 8
>> and 9.
>>
> I introduced this bug in URLClassPath.c because I copied the code from
> ClassLoader.c, which had the same pattern, and has been reported in
> https://bugs.openjdk.java.net/browse/JDK-6952230 (5 years!)
>
> - Ioi
>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Jiangli
>>>
>>>
>>>> On Nov 30, 2015, at 5:13 PM, Calvin Cheung
>>>> <[hidden email]> wrote:
>>>>
>>>> Please review this small fix for adding the check for JNI pending
>>>> exception in URLClassPath.c.
>>>>
>>>> This fix is only for jdk8u and isn't applicable to 9.
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
>>>>       (unfortunately it is a closed bug)
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/
>>>>
>>>> Tests:
>>>>     JPRT with -testset hotspot
>>>>     RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick
>>>>
>>>> thanks,
>>>> Calvin
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

Jiangli Zhou
Hi Calvin,

Looks ok.

Thanks,
Jiangli

> On Dec 1, 2015, at 1:05 PM, Calvin Cheung <[hidden email]> wrote:
>
> Hi Jiangli, David, Ioi,
>
> Thank you for the review.
>
> Here's an updated webrev:
> http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.01/
>
> Basically, the JNU_ThrowOutOfMemoryError call has been replaced with a comment.
>
> thanks,
> Calvin
>
> On 11/30/15, 8:52 PM, Ioi Lam wrote:
>>
>>
>> On 11/30/15 8:21 PM, David Holmes wrote:
>>> On 1/12/2015 12:10 PM, Jiangli Zhou wrote:
>>>> Hi Calvin,
>>>>
>>>> I see getUTF() throws OutOfMemoryError if malloc fails before returning NULL. Do you know if there is any case that getUTF() returns NULL without throwing any exception? The usage of getUTF() and exception handling don’t seem to be consistent in existing idk code. Some places check the NULL return value without throwing a new exception, while the other places throws a new OOM without checking for pending exception (like the code you are fixing). Maybe the callee does not need to re-throw the exception for the NULL return value case, and you can add a comment saying “an OutOfMemoryError” has already been thrown by getUTF()?
>>>
>>> According to classloader.c:
>>>
>>> /* Convert java string to UTF char*. Use local buffer if possible,
>>>   otherwise malloc new memory. Returns null IFF malloc failed. */
>>>
>>> So NULL implies OOME has been thrown. I'd suggest removing the new code and the JNU_ThrowOutOfMemory with a comment as Jiangli suggested.
>>>
>>> Also note we have the same kinds of patterns in classLoader.c in 8 and 9.
>>>
>> I introduced this bug in URLClassPath.c because I copied the code from ClassLoader.c, which had the same pattern, and has been reported in https://bugs.openjdk.java.net/browse/JDK-6952230 (5 years!)
>>
>> - Ioi
>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>>>> On Nov 30, 2015, at 5:13 PM, Calvin Cheung <[hidden email]> wrote:
>>>>>
>>>>> Please review this small fix for adding the check for JNI pending exception in URLClassPath.c.
>>>>>
>>>>> This fix is only for jdk8u and isn't applicable to 9.
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
>>>>>      (unfortunately it is a closed bug)
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/
>>>>>
>>>>> Tests:
>>>>>    JPRT with -testset hotspot
>>>>>    RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

David Holmes
In reply to this post by Calvin Cheung
Looks good.

Thanks,
David

On 2/12/2015 7:05 AM, Calvin Cheung wrote:

> Hi Jiangli, David, Ioi,
>
> Thank you for the review.
>
> Here's an updated webrev:
> http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.01/
>
> Basically, the JNU_ThrowOutOfMemoryError call has been replaced with a
> comment.
>
> thanks,
> Calvin
>
> On 11/30/15, 8:52 PM, Ioi Lam wrote:
>>
>>
>> On 11/30/15 8:21 PM, David Holmes wrote:
>>> On 1/12/2015 12:10 PM, Jiangli Zhou wrote:
>>>> Hi Calvin,
>>>>
>>>> I see getUTF() throws OutOfMemoryError if malloc fails before
>>>> returning NULL. Do you know if there is any case that getUTF()
>>>> returns NULL without throwing any exception? The usage of getUTF()
>>>> and exception handling don’t seem to be consistent in existing idk
>>>> code. Some places check the NULL return value without throwing a new
>>>> exception, while the other places throws a new OOM without checking
>>>> for pending exception (like the code you are fixing). Maybe the
>>>> callee does not need to re-throw the exception for the NULL return
>>>> value case, and you can add a comment saying “an OutOfMemoryError”
>>>> has already been thrown by getUTF()?
>>>
>>> According to classloader.c:
>>>
>>> /* Convert java string to UTF char*. Use local buffer if possible,
>>>    otherwise malloc new memory. Returns null IFF malloc failed. */
>>>
>>> So NULL implies OOME has been thrown. I'd suggest removing the new
>>> code and the JNU_ThrowOutOfMemory with a comment as Jiangli suggested.
>>>
>>> Also note we have the same kinds of patterns in classLoader.c in 8
>>> and 9.
>>>
>> I introduced this bug in URLClassPath.c because I copied the code from
>> ClassLoader.c, which had the same pattern, and has been reported in
>> https://bugs.openjdk.java.net/browse/JDK-6952230 (5 years!)
>>
>> - Ioi
>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>>>> On Nov 30, 2015, at 5:13 PM, Calvin Cheung
>>>>> <[hidden email]> wrote:
>>>>>
>>>>> Please review this small fix for adding the check for JNI pending
>>>>> exception in URLClassPath.c.
>>>>>
>>>>> This fix is only for jdk8u and isn't applicable to 9.
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
>>>>>       (unfortunately it is a closed bug)
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/
>>>>>
>>>>> Tests:
>>>>>     JPRT with -testset hotspot
>>>>>     RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>
>>