[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-04-07 7:05:57
Message-ID: 53424E55.1020907 () oracle ! com
[Download RAW message or body]

On 4/3/14 2:31 AM, Staffan Larsen wrote:
> Thanks Serguei,
>
> I don’t think it is necessary to initialize ‘end’ since strtol will always set it.

Ok.

>
> I still need an official Reviewer for this change.

I'm not an official reviewer yet.


Thanks,
Serguei

>
> Thanks,
> /Staffan
>
> On 28 mar 2014, at 07:21, serguei.spitsyn@oracle.com wrote:
>
>> 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]