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

Schmidt, Lutz lutz.schmidt at sap.com
Fri Dec 16 08:38:43 UTC 2016 

Thank you, Gerard!

Lutz



On 15.12.16, 16:39, "Gerard Ziemski" <gerard.ziemski at oracle.com> wrote:

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

More information about the hotspot-runtime-dev mailing list