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

Daniel D. Daugherty daniel.daugherty at oracle.com
Wed Jul 1 16:42:51 UTC 2015


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


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

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
>



More information about the hotspot-runtime-dev mailing list