Date: Sun, 15 Apr 2007 12:29:26 +0400 From: Stanislav Sedov <stas@FreeBSD.org> To: Bruce Evans <bde@zeta.org.au> Cc: cvs-src@freebsd.org, pav@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org, "Simon L. Nielsen" <simon@freebsd.org> Subject: Fw: Re: cvs commit: src/contrib/top top.X top.c top.h src/usr.bin/top machine.c Message-ID: <20070415122926.a1b0ed46.stas@FreeBSD.org>
next in thread | raw e-mail | index | archive | help
--Signature=_Sun__15_Apr_2007_12_29_26_+0400_f8scMTifxtccmOai
Content-Type: multipart/mixed;
boundary="Multipart=_Sun__15_Apr_2007_12_29_26_+0400_m/jNIVDiXUj32.PK"
--Multipart=_Sun__15_Apr_2007_12_29_26_+0400_m/jNIVDiXUj32.PK
Content-Type: text/plain; charset=US-ASCII
Content-Disposition: inline
Content-Transfer-Encoding: 7bit
Begin forwarded message:
Date: Sat, 14 Apr 2007 21:28:36 +0400
From: Stanislav Sedov <stas@FreeBSD.org>
To: Ruslan Ermilov <ru@freebsd.org>
Cc: Alfred Perlstein <alfred@freebsd.org>
Subject: Re: cvs commit: src/contrib/top top.X top.c top.h
src/usr.bin/top machine.c
On Sat, 14 Apr 2007 21:14:39 +0400
Ruslan Ermilov <ru@freebsd.org> mentioned:
> On Sat, Apr 14, 2007 at 06:46:27PM +0400, 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.
> > >
> >
> > 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.
> >
> You cannot overwrite the code due to hardware page-level
> protection.
>
Yeah, i've forgot about it.
The patch attached makes top display buffers allocation fully dynamic,
so the will be no bogus limit of 128 characters and it will scale to the
entire available width.
Also, I've added additional missed checks for NULL pointers after
malloc.
Alfred, can you review?
PS: the "top" internal structure is still a bit ugly, probably it
requires a major rewrite. I could work on this, if nobody minds.
--
Stanislav Sedov
ST4096-RIPE
--
Stanislav Sedov
ST4096-RIPE
--Multipart=_Sun__15_Apr_2007_12_29_26_+0400_m/jNIVDiXUj32.PK
Content-Type: text/x-diff;
name="top.diff"
Content-Disposition: attachment;
filename="top.diff"
Content-Transfer-Encoding: quoted-printable
Index: usr.bin/top/machine.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /home/ncvs/src/usr.bin/top/machine.c,v
retrieving revision 1.79
diff -u -r1.79 machine.c
--- usr.bin/top/machine.c 14 Apr 2007 10:16:52 -0000 1.79
+++ usr.bin/top/machine.c 14 Apr 2007 17:16:45 -0000
@@ -647,8 +647,6 @@
return ((caddr_t)&handle);
}
=20
-static char fmt[128]; /* static area where result is built */
-
char *
format_next_process(caddr_t handle, char *(*get_userid)(int), int flags)
{
@@ -662,9 +660,20 @@
struct rusage ru, *rup;
long p_tot, s_tot;
char *proc_fmt, thr_buf[6];
- char *cmdbuf =3D NULL;
+ size_t cmdlen;
+ char *cmdbuf =3D NULL, *fmt;
char **args;
=20
+ if (screen_width <=3D 0)
+ return NULL;
+
+ /* Allocate buffer to store the result */
+ fmt =3D (char *)malloc(screen_width + 1);
+ if (fmt =3D=3D NULL) {
+ warn("malloc(%d)", screen_width + 1);
+ return NULL;
+ }
+
/* find and remember the next proc structure */
hp =3D (struct handle *)handle;
pp =3D *(hp->next_proc++);
@@ -726,25 +735,31 @@
break;
}
=20
- cmdbuf =3D (char *)malloc(cmdlengthdelta + 1);
+ /* Find maximum command line lenght */
+ if (screen_width > cmdlengthdelta)
+ cmdlen =3D screen_width - cmdlengthdelta + 100;
+ else
+ cmdlen =3D 0;
+
+ cmdbuf =3D (char *)malloc(cmdlen + 1);
if (cmdbuf =3D=3D NULL) {
- warn("malloc(%d)", cmdlengthdelta + 1);
+ warn("malloc(%d)", cmdlen + 1);
return NULL;
}
=20
if (!(flags & FMT_SHOWARGS)) {
- snprintf(cmdbuf, cmdlengthdelta, "%s", pp->ki_comm);
+ snprintf(cmdbuf, cmdlen, "%s", pp->ki_comm);
}
else if (pp->ki_args =3D=3D NULL ||
- (args =3D kvm_getargv(kd, pp, cmdlengthdelta)) =3D=3D NULL || !(*args=
))
- snprintf(cmdbuf, cmdlengthdelta, "[%s]", pp->ki_comm);
+ (args =3D kvm_getargv(kd, pp, cmdlen)) =3D=3D NULL || !(*args))
+ snprintf(cmdbuf, cmdlen, "[%s]", pp->ki_comm);
else {
char *src, *dst, *argbuf;
char *cmd;
size_t argbuflen;
size_t len;
=20
- argbuflen =3D cmdlengthdelta * 4;
+ argbuflen =3D cmdlen * 4;
argbuf =3D (char *)malloc(argbuflen + 1);
if (argbuf =3D=3D NULL) {
warn("malloc(%d)", argbuflen + 1);
@@ -777,10 +792,10 @@
*dst =3D '\0';
=20
if (strcmp(cmd, pp->ki_comm) !=3D 0 )
- snprintf(cmdbuf, cmdlengthdelta, "%s (%s)",argbuf, \
+ snprintf(cmdbuf, cmdlen, "%s (%s)",argbuf, \
pp->ki_comm);
else
- strlcpy(cmdbuf, argbuf, cmdlengthdelta);
+ strlcpy(cmdbuf, argbuf, cmdlen);
=20
free(argbuf);
}
@@ -802,7 +817,7 @@
p_tot =3D rup->ru_inblock + rup->ru_oublock + rup->ru_majflt;
s_tot =3D total_inblock + total_oublock + total_majflt;
=20
- sprintf(fmt, io_Proc_format,
+ snprintf(fmt, screen_width, io_Proc_format,
pp->ki_pid,
namelength, namelength, (*get_userid)(pp->ki_ruid),
rup->ru_nvcsw,
@@ -812,9 +827,7 @@
rup->ru_majflt,
p_tot,
s_tot =3D=3D 0 ? 0.0 : (p_tot * 100.0 / s_tot),
- screen_width > cmdlengthdelta ?
- screen_width - cmdlengthdelta : 0,
- printable(cmdbuf));
+ cmdlen, printable(cmdbuf));
=20
free(cmdbuf);
=20
@@ -829,7 +842,7 @@
snprintf(thr_buf, sizeof(thr_buf), "%*d ",
sizeof(thr_buf) - 2, pp->ki_numthreads);
=20
- sprintf(fmt, proc_fmt,
+ snprintf(fmt, screen_width, proc_fmt,
pp->ki_pid,
namelength, namelength, (*get_userid)(pp->ki_ruid),
thr_buf,
@@ -841,8 +854,7 @@
smpmode ? pp->ki_lastcpu : 0,
format_time(cputime),
ps.wcpu ? 100.0 * weighted_cpu(pct, pp) : 100.0 * pct,
- screen_width > cmdlengthdelta ? screen_width - cmdlengthdelta : 0,
- printable(cmdbuf));
+ cmdlen, printable(cmdbuf));
=20
free(cmdbuf);
=20
Index: contrib/top/display.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /home/ncvs/src/contrib/top/display.c,v
retrieving revision 1.9
diff -u -r1.9 display.c
--- contrib/top/display.c 19 May 2005 13:34:19 -0000 1.9
+++ contrib/top/display.c 14 Apr 2007 17:16:46 -0000
@@ -68,6 +68,10 @@
static char **memory_names;
static char **swap_names;
=20
+static char *procstates_buffer;
+static char *memory_buffer;
+static char *swap_buffer;
+
static int num_procstates;
static int num_cpustates;
static int num_memory;
@@ -104,14 +108,8 @@
=20
if (lines < 0)
lines =3D 0;
- /* we don't want more than MAX_COLS columns, since the machine-depende=
nt
- modules make static allocations based on MAX_COLS and we don't want
- to run off the end of their buffers */
+
display_width =3D screen_width;
- if (display_width >=3D MAX_COLS)
- {
- display_width =3D MAX_COLS - 1;
- }
=20
/* now, allocate space for the screen buffer */
screenbuf =3D (char *)malloc(lines * display_width);
@@ -142,6 +140,11 @@
/* only do the rest if we need to */
if (lines > -1)
{
+ /* allocate line buffers */
+ procstates_buffer =3D (char *)malloc(screen_width + 1);
+ memory_buffer =3D (char *)malloc(screen_width + 1);
+ swap_buffer =3D (char *)malloc(screen_width + 1);
+
/* save pointers and allocate space for names */
procstate_names =3D statics->procstate_names;
num_procstates =3D string_count(procstate_names);
@@ -174,6 +177,11 @@
}
}
=20
+ /* Check if memory was allocated */
+ if (!procstates_buffer || !memory_buffer || !swap_buffer || !lprocstat=
es ||
+ !lswap || !lcpustates || !cpustate_columns || !lmemory)
+ return (-1);
+
/* return number of lines available */
return(lines);
}
@@ -281,7 +289,6 @@
}
=20
static int ltotal =3D 0;
-static char procstates_buffer[MAX_COLS];
=20
/*
* *_procstates(total, brkdn, names) - print the process summary line
@@ -323,7 +330,6 @@
int *brkdn;
=20
{
- static char new[MAX_COLS];
register int i;
=20
/* update number of processes only if it has changed */
@@ -358,10 +364,16 @@
/* see if any of the state numbers has changed */
if (memcmp(lprocstates, brkdn, num_procstates * sizeof(int)) !=3D 0)
{
+ char *new =3D (char *)malloc(display_width + 1);
+ if (new =3D=3D NULL) {
+ warn("malloc(%d)", display_width + 1);
+ return;
+ }
/* format and update the line */
summary_format(new, brkdn, procstate_names);
line_update(procstates_buffer, new, x_brkdn, y_brkdn);
memcpy(lprocstates, brkdn, num_procstates * sizeof(int));
+ free(new);
}
}
=20
@@ -516,8 +528,6 @@
* for i_memory ONLY: cursor is on the previous line
*/
=20
-char memory_buffer[MAX_COLS];
-
i_memory(stats)
=20
int *stats;
@@ -536,11 +546,18 @@
int *stats;
=20
{
- static char new[MAX_COLS];
+
+ char *new =3D (char *)malloc(display_width + 1);
+ if (new =3D=3D NULL) {
+ warn("malloc(%d)", display_width + 1);
+ return;
+ }
=20
/* format the new line */
summary_format(new, stats, memory_names);
line_update(memory_buffer, new, x_mem, y_mem);
+
+ free(new);
}
=20
/*
@@ -550,8 +567,6 @@
* for i_swap ONLY: cursor is on the previous line
*/
=20
-char swap_buffer[MAX_COLS];
-
i_swap(stats)
=20
int *stats;
@@ -570,11 +585,17 @@
int *stats;
=20
{
- static char new[MAX_COLS];
+
+ char *new =3D (char *)malloc(display_width + 1);
+ if (new =3D=3D NULL) {
+ warn("malloc(%d)", display_width + 1);
+ return;
+ }
=20
/* format the new line */
summary_format(new, stats, swap_names);
line_update(swap_buffer, new, x_swap, y_swap);
+ free(new);
}
=20
/*
@@ -713,6 +734,9 @@
register char *p;
register char *base;
=20
+ if (thisline =3D=3D NULL)
+ return;
+
/* make sure we are on the correct line */
while (lastline < y_procs + line)
{
@@ -732,6 +756,8 @@
=20
/* zero fill the rest of it */
memzero(p, display_width - (p - base));
+
+ free(thisline);
}
=20
u_process(line, newline)
@@ -744,6 +770,9 @@
register int screen_line =3D line + Header_lines;
register char *bufferline;
=20
+ if (newline =3D=3D NULL)
+ return;
+
/* remember a pointer to the current line in the screen buffer */
bufferline =3D &screenbuf[lineindex(line)];
=20
@@ -779,6 +808,9 @@
{
line_update(bufferline, newline, 0, line + Header_lines);
}
+
+ free(newline);
+
}
=20
u_endscreen(hi)
--Multipart=_Sun__15_Apr_2007_12_29_26_+0400_m/jNIVDiXUj32.PK--
--Signature=_Sun__15_Apr_2007_12_29_26_+0400_f8scMTifxtccmOai
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (FreeBSD)
iD8DBQFGIeJmK/VZk+smlYERAo1jAJ0Wg6tJlmrW8cHHYcy3BTuHzqcgJQCdFkpm
dh3o0OsrtmlCgECJWl9WsAw=
=f/2U
-----END PGP SIGNATURE-----
--Signature=_Sun__15_Apr_2007_12_29_26_+0400_f8scMTifxtccmOai--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070415122926.a1b0ed46.stas>