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

[David Holmes]

On 19/01/2012 9:49 PM, Frederic Parain wrote:
> On 01/19/12 12:39 PM, Robert Ottenhag wrote:
>> However, I do not get it, why cannot ordinary strcasecmp(str, "true") be
>> used here, is it problem with an input string that is not null
>> terminated, or is it some other problem?
>
> Correct. The parsing must be applied only to a section of str which
> is not null terminated.
>
>> Reading the bug, the failing input is "-all=true", but how is that a
>> problem, assuming the upper layer code has separated "-all=" from "true"
>> before parsing the value?
>>
>
> The CR is missing some information (I've received a more detailed
> description of the failure by e-mail).
>
> The failing input is in fact "-all=true filename.hprof"
>
> Where the value of the boolean argument is followed by
> a delimiter (a space in this case) and another argument.
>
> When the parsing method is invoked, str points to the 't' of
> "true" and len is 4:
>
> Using strcasecmp:
> strcasecmp("true filename.hprof","true") returns a positive number
>
> Using strncasecmp:
> strncasecmp("true filename.hprof","true",len) returns zero

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 }

this doesn't limit itself to checking only len characters in str.

David
-----


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