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

[Dmitry Dmitriev]

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

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