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

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8038296 sun/tools/jinfo/Basic.sh: java.io.IOException: Command failed in target VM
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2014-03-28 6:21:40
Message-ID: 533514F4.8050906 () oracle ! com
[Download RAW message or body]

On 3/27/14 12:55 AM, Staffan Larsen wrote:
> Here is an updated webrev which incorporates Dmitry’s feedback:
>
> http://cr.openjdk.java.net/~sla/8038296/webrev.01/


It looks good.
The only suggestion is to initialize the 'end':
   char* end;

Not sure what value is better to use for initialization.
Probably, end = probe would work Ok.

Thanks,
Serguei
>
> Thanks,
> /Staffan
>
> On 25 mar 2014, at 19:36, Dmitry Samersoff <dmitry.samersoff@oracle.com> wrote:
>
>> Staffan,
>>
>>> Yes, that will find problems when trying to convert something like
>>> ‘bla’. It will not capture the case where the input string is a too
>>> large (or small) value (value < LONG_MIN or > LONG_MAX). To capture
>>> both cases it looks like we need something like:
>>>   errno = 0;
>>>   char* end;
>>>   int probe_typess = (int) strtol(probe, &end, 10);
>>>   if (end == probe || errno) {
>>>     return JNI_ERR;
>>>   }
>> As probe_typess is positive and you are converting long to int
>> It's be better to check value boundaries explicitly:
>>
>>    char* end;
>>    long ptl = strtol(probe, &end, 10);
>>    if (end == probe || ptl < 0 || ptl > MAX_INT) {
>>      return JNI_ERR;
>>    }
>>
>>    int probe_typess = (int) ptl;
>>
>>
>> -Dmitry
>>
>>
>> On 2014-03-25 17:35, Staffan Larsen wrote:
>>> On 25 mar 2014, at 13:46, Dmitry Samersoff
>>> <dmitry.samersoff@oracle.com> wrote:
>>>
>>>> Staffan,
>>>>
>>>> strtol() sets errno in two cases -
>>>>
>>>> ERANGE if supplied value is less then LONG_MIN or greater than
>>>> LONG_MAX EINVAL if supplied base is not supported.
>>>>
>>>> if you pass probe == 'bla', strtol just return 0 and errno will not
>>>> be set. So I'm not sure that check for errno has any value here.
>>>>
>>>> One of possible way to check that supplied value is convertible to
>>>> long is
>>>>
>>>> char *eptr = probe; strtol(probe, (char **)&eptr, 10); if (eptr ==
>>>> probe) { // we can't convert supplied value return JNI_ERR; }
>>> Yes, that will find problems when trying to convert something like
>>> ‘bla’. It will not capture the case where the input string is a too
>>> large (or small) value (value < LONG_MIN or > LONG_MAX). To capture
>>> both cases it looks like we need something like:
>>>
>>> errno = 0; char* end; int probe_typess = (int) strtol(probe, &end,
>>> 10); if (end == probe || errno) { return JNI_ERR; }
>>>
>>>
>>> /Staffan
>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2014-03-25 11:31, Staffan Larsen wrote:
>>>>> attachListener_solaris.cpp calls atoi() and then checks errno to
>>>>> see if any errors occurred. The problem is that atoi() does not
>>>>> set errno, so some old value of errno is checked which sometimes
>>>>> causes the function to fail.
>>>>>
>>>>> The fix is to replace atoi() with strtol() which does set errno.
>>>>> But errno is only set if an error occurred and not set to 0 in
>>>>> case of success. Thus, I set errno to 0 before calling strtol()
>>>>> and check the value afterwards.
>>>>>
>>>>> Verified with a JPRT run.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~sla/8038296/webrev.00/ bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8038296
>>>>>
>>>>> Thanks, /Staffan
>>>>>
>>>>
>>>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg,
>>>> Russia * I would love to change the world, but they won't give me
>>>> the sources.
>>
>> -- 
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.

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