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

[Frederic Parain]

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

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