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

[Schmidt, Lutz]

Hi Gerard,

thank you for your comments and for offering your sponsorship. I gladly
accept your offer. 

I left the code unchanged (as per Davids request), except for the
comparison you mentioned. I now reads
   if (nameLen < maxFlagLen) {


in the new (final?) iteration of the webrev:
http://cr.openjdk.java.net/~simonis/webrevs/2016/8170981.v3/

Thanks again to all involved!
Lutz




On 14.12.16, 21:59, "David Holmes" <david.holmes at oracle.com> wrote:

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