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

[Frederic Parain]

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.
Unless someone as the bad idea to use a digit character
as delimiter, it should be safe.

However, I'm not sure that trailing characters are properly
detected. Do you consider that as a show stopper?

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