cvs commit: src/contrib/top top.X top.c top.h src/usr.bin/top machine.c

[Stanislav Sedov]

On Sun, 15 Apr 2007 17:25:15 +1000 (EST)
Bruce Evans <bde at zeta.org.au> mentioned:

> On Sat, 14 Apr 2007, Stanislav Sedov wrote:
>
> > On Sat, 14 Apr 2007 18:17:30 +0400
> > Stanislav Sedov <stas at FreeBSD.org> mentioned:
> >
> >> Well, not quite right. If you screen is wider then 128 symbols, there
> >> could be an overflow, since the row buffer is 128 bytes length.
> >>
> >> I have not touched any limits, just replaced the string it displays. So
> >> there can be overflow with patch or without it, if both the command
> >> name and screen width is wider then 128.
>
> It couldn't overflow before, because the command name width was limited
> by sizeof(pp->ki_comm) = 20, and other field widths are mostly fixed and
> must add up to less than about 60 to leave space for almost 20-character
> command names on 80-column terminals.
>
> > That's even better. In contrib src display.c it always NULL-terminate
> > string at the screen_width point, so it's always write '\0' to the
> > unknown area if screen_width is larger than 128. Hopefully, the storage
> > is allocated last in .data section, so it doesn't overwrite any
> > important data and code.
>
> No, it NUL-terminates at display_width = min(screen_width, MAX_COLS - 1).
> display_width's only reason for existence is to cache the result of
> this min() so as to do the truncation efficiently as well as safely.
>

Yeah, already discovered too.

> MAX_COLS is bogusly named.  It is the buffer size for various buffers. It
> is certainly not the maximum nomber of columns, but is effectively 1
> less than that, since if the physical display width is > MAX_COLS - 1
> then the truncation has the non-null effect of limiting all output to
> MAX_COLS - 1 columns.
>
> screen_width is slightly bogusly named.  It is normally the physical
> screen width less 1.  Here 1 is subtracted to avoid problems with line
> wrap, so this name is OK if you think of the screen width as being the
> usable screen width, but that width as actually given by the
> display_width variable so the screen_width variable shouldn't be used
> for it too.  I think screen_width should be the physical screen width
> and private to screen.c and maybe display.c.  The only correct use of
> screen_width is currently to initialize display_width from it.  Other
> uses give the buffer overrun in top.c and completely wrong right
> justification of some things in display.c in cases where
> screen_width != display_width.  Right justification to display_width
> would be less wrong.
>
> The interaction of the subtraction of 1 from screen_width with the
> subtraction of 1 from MAX_COLS is a bit confusing and gives potential
> buffer overrun for the default initialization of screen_width to
> MAX_COLS (should be to MAX_COLS - 1).  This initialization is used if
> top can't figure out the terminal width.  Then it has the same effect
> as a terminal of width MAX_COLS + 1 -- there is no problem in display.c
> since display_width = MAX_COLS - 1 is used there, but the printf() in
> top.c can overrun by 1 now that the command name width can be large.
>

Thanks for deep analysis, I'll try to optimise and cleanup the code to
make it more smart. BTW, I've modified it to use dynamically allocated
buffers (I've forgotten to CC it to cvs at freebsd.org). It allocate
buffers for cached strings of (screen_width + 1) bytes length now to be
able to store the full screen line and terminating NULL.

Can you review, please?

--
Stanislav Sedov
ST4096-RIPE
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20070415/278c6140/attachment.pgp