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

[David Holmes]

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.

Please add a trailing comment here:

480     char spaces[] = "          ";  // 10 spaces plus NUL

Style nit:

481     const unsigned int maxFlagLen = 50;
482     const unsigned int nSpaces    = strlen(spaces);

no reason to align the =.

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

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