RFR (S): 8129786: Buffer overrun when passing long not existing option in JDK 9

[David Holmes]

Hi Dmitry,

Looks good to me. If you prepare the changeset and send it to me I'll do 
the JPRT submit for you.

Thanks,
David

On 2/07/2015 2:38 AM, Dmitry Dmitriev wrote:
> Hello Dan,
>
> Thank you for review! Updated webrev:
> http://cr.openjdk.java.net/~ddmitriev/8129786/webrev.01/
> <http://cr.openjdk.java.net/%7Eddmitriev/8129786/webrev.01/>
> Please see my comments inline.
>
> Regards,
> Dmitry
>
> On 01.07.2015 16:49, Daniel D. Daugherty wrote:
>> > Webrev: http://cr.openjdk.java.net/~ddmitriev/8129786/webrev.00/
>>
>> src/share/vm/runtime/arguments.cpp
>>     L840:   if (arg_len <= BUFLEN) {
>>         Please add a comment. Perhaps:
>>
>>         // Only make the obsolete check for valid arguments.
> Done!
>>
>>     L843:     strncpy(stripped_argname, argname, arg_len);
>>     L844:     stripped_argname[arg_len] = '\0'; //strncpy doesn't null
>> terminate.
>>         This is not due to your change but this comment isn't quite
>> right.
>>         Perhaps:
>>
>>             stripped_argname[arg_len] = '\0';  // strncpy may not null
>> terminate.
>>
>>         strncpy() null terminates if the length of 'argname' is
>>         less than arg_len. Also added one more space after ';'
>>         and added a space after '//'.
> Done!
>>
>>     L847:       char version[256];
>>         Can you change this '256' to BUFLEN+1 also?
> I prefer to leave it as is, because there are no relationship between
> 'version' and BUFLEN. BUFLEN is used for parsing options, thus I use it
> for stripped_argname. But I think it will be better not to use it for
> 'version'.
>
>>
>> test/runtime/CommandLine/TestLongUnrecognizedVMOption.java
>>     L27:  * @summary Verify that JVM correctly process very long
>> unregnized VM option
>>         Typo: 'process' -> 'processes'
>>         Typo: 'unregnized' -> 'unrecognized'
>>
> Done!
>> L29:  * @modules java.management
>>         Why this module?
> I got this dependency from script which used to find dependencies of
> test code.
>>
>>     L49: extra blank line at the end; jcheck may not like this
>>
> Done!
>> Thumbs up. I don't need to see another webrev if you
>> decide to fix these minor nits.
>>
>> Dan
>>
>>
>> On 7/1/15 6:49 AM, Dmitry Dmitriev wrote:
>>> Hello,
>>>
>>> Please review this small fix and new test. Also, I need a sponsor for
>>> this fix, who can push it.
>>>
>>> In this fix logic for stripped_argname was put into "if (arg_len <=
>>> BUFLEN)" statement. stripped_argname is used only to check is option
>>> is newly obsolete. Since valid VM option should be not bigger than
>>> 255 characters(BUFLEN value), then obsolete_jvm_flags contains only
>>> options with strlen <= BUFLEN.
>>>
>>> Webrev: http://cr.openjdk.java.net/~ddmitriev/8129786/webrev.00/
>>> <http://cr.openjdk.java.net/%7Eddmitriev/8129786/webrev.00/>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8129786
>>> Tested: JPRT(with new test), hotspot all & vm.quick
>>>
>>> Thanks,
>>> Dmitry
>