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

[Daniel D. Daugherty]

On 7/1/15 10:49 AM, Dmitry Dmitriev wrote:
> On 01.07.2015 19:42, Daniel D. Daugherty wrote:
>>
>> On 7/1/15 10: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'.
>>
>> Is there some constant hanging around that is appropriate?
>> A literal '256' is just bugging me... :-)
> Yes, I agree with you :) But I don't see any of them around.

OK...


>>
>>
>>>
>>>>
>>>> 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.
>>
>> OK, but why? I see nothing in this test that requires the
>> java.management module...
> I think that because jdk.test.lib.ProcessTools is depends on 
> java.management and this leads to the test dependency.

OK... that makes sense.

Dan

>
> Thank you,
> Dmitry
>>
>> Dan
>>
>>
>>>>
>>>>     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
>>>
>>
>