RFR(XS) 8170981: Possible access to char array with negative index

[Gerard Ziemski]

hi Schmidt,

Thank you very much for fixing this. I have 2 feedbacks:

#1 Should we have a comment explaining what “*” means in the format argument to “print”? I know I had to look it up in a man page.


#2 In this code:

+    // Make sure we do not punch a '\0' at a negative char array index.
+    unsigned int nameLen = strlen(_name);
+    if (nameLen <= maxFlagLen) {
+      spaces[maxFlagLen - MAX2(maxFlagLen-nSpaces, nameLen)] = '\0';
     st->print("%s", spaces);

+    }

shouldn’t:

if (nameLen <= maxFlagLen) {

be:

if (nameLen < maxFlagLen) {

Otherwise if “nameLen==50", we enter the “if” and do "st->print("%s", spaces);” with ‘\0’ for spaces, which is unneeded NOP.


I can sponsor this fix for you if you wish. This should go into jdk10 despite it being marked for jdk9, right?



cheers

> On Dec 13, 2016, at 7:24 AM, Schmidt, Lutz <lutz.schmidt at sap.com> wrote:
> 
> Hi David, 
> 
> thanks for the review. Please find my responses below, next to your
> comments. 
> 
> About your question “What’s that code doing after all?”: I can only try to
> understand and describe what the original author tried to accomplish. Here
> we go:
> 
> Flag names are assumed (better: hoped) to have a length of at most 40
> characters. The “=“ sign is aligned according to that assumption. The
> author obviously did not want to move the ‘=‘ further to the right to
> accommodate for just the few flags with names longer than 40 characters.
> On the other hand, the author wanted to keep the alignment of subsequent
> columns intact. So he introduced an elastic buffer (10 spaces) which is
> printed after the flag value. For each flag name character in excess of
> 40, one space is trimmed from that elastic buffer.
> 
> There is a new iteration (just the comment and alignment adjustments) of
> the webrev:
>    http://cr.openjdk.java.net/~simonis/webrevs/2016/8170981.v2/
> 
> 
> Regards,
> Lutz
> 
> 
> 
> On 12.12.16, 23:19, "David Holmes" <david.holmes at oracle.com> wrote:
> 
>> Hi Lutz,
>> 
>> On 13/12/2016 3:10 AM, Schmidt, Lutz wrote:
>>> Rachel,
>>> 
>>> 
>>> thanks a lot for the review. An updated webrev (according to my comments
>>> below) can be found here:
>>>   http://cr.openjdk.java.net/~simonis/webrevs/2016/8170981.v1
>> 
>> I have a few nits with this:
>> 
>> 476     // nSpaces spaces below to adjust the space between the flag
>> value and the
>> 
>> The array is still hardwired to 10 spaces so the comment should still
>> say 10 IMHO - otherwise it sounds like the number of space is
>> dynamically determined.
> 
> OK. Concern understood. Comment changed back to 10.
> 
>> 
>> Please add a trailing comment here:
>> 
>> 480     char spaces[] = "          ";  // 10 spaces plus NUL
> 
> OK. Comment added.
> 
>> 
>> Style nit:
>> 
>> 481     const unsigned int maxFlagLen = 50;
>> 482     const unsigned int nSpaces    = strlen(spaces);
>> 
>> no reason to align the =.
> 
> My reason: personal aesthetics. OpenJDK code is not uniform in that
> respect. Alignment removed.
> 
>> 
>> 484     st->print("%9s %-*s = ", _type, maxFlagLen-nSpaces, _name);
>> 
>> I'm not quite sure exactly how this code tries to print different length
>> flag names. The above will left-justify and space-pad to at least 40
>> characters IIUC. If strlen(_name) is < 40 then we later add more padding
>> (up to 10) from the spaces array. Why do we need to do that? Why don't
>> we simply use %-50s in the first place ??
> 
> See explanation above. Padding supposed to happen behind the flag value.
> 
>> 
>> Thanks,
>> David
>> 
>>> Regards,
>>> Lutz
>>> 
>>> 
>>> On 12.12.16, 17:07, "hotspot-runtime-dev on behalf of Rachel Protacio"
>>> <hotspot-runtime-dev-bounces at openjdk.java.net on behalf of
>>> rachel.protacio at oracle.com> wrote:
>>> 
>>>> Hi,
>>>> 
>>>> I wonder whether it's better to declare
>>>> 
>>>>   const char* spaces = "         ";
>>>> 
>>>> and then set nSpaces as strlen(spaces), so it's clear that the spaces
>>>> string will always be consistent with nSpaces?
>>> 
>>> Good idea, Rachel! I feel uncomfortable with the ³disconnected²
>>> declarations as well. I had to use a non-constant char array because a
>>> Œ\0¹ is stored into it.
>>> 
>>> Btw, a ³beautiful² fix would abandon the spaces array altogether and
>>> implement columnar alignment with the help of tty->fill_to(pos). But
>>> that
>>> would be a reengineering effort which might not take the Java9 hurdle.
>>> 
>>>> 
>>>> Otherwise, looks good to me.
>>>> Rachel
>>>> 
>>>> On 12/12/2016 9:45 AM, Schmidt, Lutz wrote:
>>>>> Hi,
>>>>> 
>>>>> may I please ask for reviews and sponsorship for this small fix in
>>>>> command
>>>>> line flag printing?
>>>>> 
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170981
>>>>> webrev: http://cr.openjdk.java.net/~simonis/webrevs/2016/8170981
>>>>> 
>>>>> Thank you!
>>>>> Lutz