RFR (XS) : 7131346 Parsing of boolean arguments to diagnostic commands is broken

[Frederic Parain]

Thanks David,

I just need a second reviewer and I'll push the fix.

Fred

On 1/20/12 12:18 PM, David Holmes wrote:
> On 19/01/2012 10:33 PM, Frederic Parain wrote:
>> On 01/19/12 01:28 PM, David Holmes wrote:
>>>> However, I'm not sure that trailing characters are properly
>>>> detected. Do you consider that as a show stopper?
>>>
>>> Depends on exactly what you mean by that. If extra junk characters are
>>> ignored rather than causing an error that's okay to me.
>>
>> They will be silently ignored.
>
> Not a showstopper in that case.
>
> So this looks good to go.
>
> David
> -----
>
>> Fred
>>
>>>> Fred
>>>>
>>>>>> Fred
>>>>>>
>>>>>>> On 01/19/2012 11:48 AM, Dmitry Samersoff wrote:
>>>>>>>> Frederic,
>>>>>>>>
>>>>>>>> Sorry, of course
>>>>>>>>
>>>>>>>> strncasecmp(str, "true", 4) == 0&& str[4] == 0
>>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>> On 2012-01-19 14:26, Frederic Parain wrote:
>>>>>>>>> strncasecmp(str, "true", 4) == 0 would accept
>>>>>>>>> arguments like this:
>>>>>>>>>
>>>>>>>>> -all=truefalse
>>>>>>>>>
>>>>>>>>> which are not valid.
>>>>>>>>>
>>>>>>>>> Fred
>>>>>>>>>
>>>>>>>>> On 01/19/12 11:22 AM, Dmitry Samersoff wrote:
>>>>>>>>>> Frederic,
>>>>>>>>>>
>>>>>>>>>> I think explicit check for len is not necessary,
>>>>>>>>>>
>>>>>>>>>> strncasecmp(str, "true", 4) == 0
>>>>>>>>>>
>>>>>>>>>> is enough.
>>>>>>>>>>
>>>>>>>>>> -Dmitry
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2012-01-19 13:59, Frederic Parain wrote:
>>>>>>>>>>> This is a small fix (2 lines) to fix an issue with the
>>>>>>>>>>> parsing of boolean arguments by diagnostic commands
>>>>>>>>>>>
>>>>>>>>>>> CR is not available on bugs.sun.com yet, but the description
>>>>>>>>>>> says that the string comparisons to identify "true" or "false"
>>>>>>>>>>> values doesn't take into account the length of the argument
>>>>>>>>>>> being parse.
>>>>>>>>>>>
>>>>>>>>>>> The suggested fix is:
>>>>>>>>>>>
>>>>>>>>>>> --- old/src/share/vm/services/diagnosticArgument.cpp Thu Jan 19
>>>>>>>>>>> 10:36:10 2012
>>>>>>>>>>> +++ new/src/share/vm/services/diagnosticArgument.cpp Thu Jan 19
>>>>>>>>>>> 10:36:10 2012
>>>>>>>>>>> @@ -62,9 +62,9 @@
>>>>>>>>>>> if (len == 0) {
>>>>>>>>>>> set_value(true);
>>>>>>>>>>> } else {
>>>>>>>>>>> - if (strcasecmp(str, "true") == 0) {
>>>>>>>>>>> + if (len == strlen("true")&& strncasecmp(str, "true", len) ==
>>>>>>>>>>> 0) {
>>>>>>>>>>> set_value(true);
>>>>>>>>>>> - } else if (strcasecmp(str, "false") == 0) {
>>>>>>>>>>> + } else if (len == strlen("false")&& strncasecmp(str, "false",
>>>>>>>>>>> len)
>>>>>>>>>>> == 0) {
>>>>>>>>>>> set_value(false);
>>>>>>>>>>> } else {
>>>>>>>>>>> THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~fparain/7131346/webrev.00/
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Fred
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com