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

[David Holmes]

Hi Gerard,

On 15/12/2016 3:01 AM, Gerard Ziemski wrote:
> 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.

No. Explanatory API notes should be very rarely needed in the code. This 
is something the developer should know or look up.

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

This is a bug fix for 9. It is only enhancements that are deferred to 10.

Thanks,
David

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