Date: Sun, 15 Apr 2007 17:25:15 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Stanislav Sedov <stas@freebsd.org> Cc: cvs-src@freebsd.org, pav@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org, "Simon L. Nielsen" <simon@freebsd.org> Subject: Re: cvs commit: src/contrib/top top.X top.c top.h src/usr.bin/top machine.c Message-ID: <20070415161007.G37658@besplex.bde.org> In-Reply-To: <20070414184627.746fa559.stas@FreeBSD.org> References: <200704141016.l3EAGqIs023798@repoman.freebsd.org> <1176546388.54822.11.camel@ikaros.oook.cz> <1176546959.54822.14.camel@ikaros.oook.cz> <20070414154246.89ad2946.stas@FreeBSD.org> <20070414124654.GB1687@zaphod.nitro.dk> <20070414181730.eca262c0.stas@FreeBSD.org> <20070414184627.746fa559.stas@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 14 Apr 2007, Stanislav Sedov wrote: > On Sat, 14 Apr 2007 18:17:30 +0400 > Stanislav Sedov <stas@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. 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. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070415161007.G37658>