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

Thu Dec 15 15:39:43 UTC 2016

hi Lutz,

Thank you for the final tweak. The change looks good.

I will run it through internal testing and push it for you once that’s successfully done.

cheers

> On Dec 15, 2016, at 4:00 AM, Schmidt, Lutz <lutz.schmidt at sap.com> wrote:
> 
> 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
>>> 
> 