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

[David Holmes]

On 19/01/2012 10:18 PM, Frederic Parain wrote:
> On 01/19/12 01:08 PM, David Holmes wrote:
>> Okay that all makes sense now.
>>
>> But does that mean other parse routines have a similar issue eg:
>>
>> 39 template <> void DCmdArgument<jlong>::parse_value(const char* str,
>> 40 size_t len, TRAPS) {
>> 41 if (sscanf(str, INT64_FORMAT, &_value) != 1) {
>> 42 THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
>> 43 "Integer parsing error in diagnostic command arguments");
>> 44 }
>> 45 }
>
> sscanf will stop parsing as soon as it finds a character
> which cannot be used to create an integer.

Ah - right!

> Unless someone as the bad idea to use a digit character
> as delimiter, it should be safe.

Yeah that would be a bad idea :)

> 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.

David
-----

> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>>
>>>
>