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

[Schmidt, Lutz]

Hi Thomas,

yes we could. But: I tried to keep the fix minimal.

Using outputStream::fill_to() at just one place wouldn’t help much. I would rather remove fixed spacing in all format strings. That converts the fix into a redesign and probably makes it unsuitable for Java9. I do have such a version, though.

Regards,
Lutz


From: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Date: Dienstag, 13. Dezember 2016 um 09:41
To: David Holmes <david.holmes at oracle.com<mailto:david.holmes at oracle.com>>
Cc: Lutz Schmidt <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>>, Rachel Protacio <rachel.protacio at oracle.com<mailto:rachel.protacio at oracle.com>>, "hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>" <hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>>
Subject: Re: RFR(XS) 8170981: Possible access to char array with negative index



On Mon, Dec 12, 2016 at 11:19 PM, David Holmes <david.holmes at oracle.com<mailto: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.

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


Also, could we not just use outputStream::fill_to() instead of putting those spaces together manually?


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<mailto:hotspot-runtime-dev-bounces at openjdk.java.net> on behalf of
rachel.protacio at oracle.com<mailto: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