>Number:         7164
>Category:       kern
>Synopsis:       procfs: procfs_cmdline.c fixes
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people (Kernel Bug People)
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Mar 15 15:35:01 1999
>Last-Modified:
>Originator:     Jaromir Dolecek
>Organization:
	Per4mance, Brno, Czech Republic
>Release:        NetBSD-19990115 or so
>Environment:
	
System: NetBSD jdolecek.per4mance.cz 1.3I NetBSD 1.3I (JDOLECEK) #7: Thu Mar 11 18:33:22 MET 1999 dolecek@jdolecek.per4mance.cz:/home/dolecek/tmp/src/sys/arch/i386/compile/JDOLECEK i386


>Description:
	There are two main problems with code in procfs_cmdline.c:
	1. it uses ARG_MAX (about 256K) big buffer to hold the data;
		as huge amount of programs have much smaller argv,
		it's greatly inefficient
	2. the argv string is mapped by one character at time

	besides that, I found out that name of SZOMB or kernel
	process is cut to 4 characters -- the second parameter
	in call to snprintf() stayed accidentaly on sizeof(arg)
	aka sizeof(char **) from time when arg was automatic array.
	It's also not necessary to call strlen() on resulting string,
	the same value can be obtained directly from snprintf() call.

>How-To-Repeat:
>Fix:
	The patch below uses PAGE_SIZE for both the buffer size
	and unit for argv copying, I assumed it's the most
	efficient value for this kind of things.
	
	The argv data are copied from position uio->uio_offset
	to the end of the repective memory page, in pythonic
	terms:
		to_write = argv[uio->uio_offset:round_page(uio->uio_offset+1)]

	I tested it with /proc/curproc/cmdline and /proc/*{1|2}/cmdline.
	It seems to work okay for processes with very long (>PAGE_SIZE)
	argv too, but I didn't stress it very hard.

Index: procfs_cmdline.c
===================================================================
RCS file: /home/dolecek/cvsroot/procfs/procfs_cmdline.c,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -r1.3 -r1.4
--- procfs_cmdline.c	1999/03/15 18:57:05	1.3
+++ procfs_cmdline.c	1999/03/15 22:53:07	1.4
@@ -48,6 +48,7 @@
 #include <miscfs/procfs/procfs.h>
 
 #include <vm/vm.h>
+#include <vm/vm_param.h>
 #if defined(UVM)
 #include <uvm/uvm_extern.h>
 #endif
@@ -63,7 +64,8 @@
 	struct uio *uio;
 {
 	struct ps_strings pss;
-	int xlen, len, count, error;
+	int xlen, count, error, i;
+	size_t len, upper_bound;
 	struct uio auio;
 	struct iovec aiov;
 	vaddr_t argv;
@@ -75,11 +77,8 @@
 
 	/*
 	 * Allocate a temporary buffer to hold the arguments.
-	 *
-	 * XXX THIS COULD BE HANDLED MUCH MORE INTELLIGENTLY,
-	 * XXX WITHOUT REQUIRING A 256k TEMPORARY BUFFER!
 	 */
-	arg = malloc(ARG_MAX, M_TEMP, M_WAITOK);
+	arg = malloc(PAGE_SIZE, M_TEMP, M_WAITOK);
 
 	/*
 	 * Zombies don't have a stack, so we can't read their psstrings.
@@ -87,8 +86,7 @@
 	 * ps(1) would display.
 	 */
 	if (p->p_stat == SZOMB || (p->p_flag & P_SYSTEM) != 0) {
-		snprintf(arg, sizeof(arg), "(%s)", p->p_comm);
-		len = strlen(arg);	/* exclude last NUL */
+		len = snprintf(arg, PAGE_SIZE, "(%s)", p->p_comm);
 		goto doio;
 	}
 
@@ -152,19 +150,20 @@
 		goto bad;
 
 	/*
-	 * Now copy in the actual argument vector, one byte at a time,
+	 * Now copy in the actual argument vector, one page at a time,
 	 * since we don't know how long the vector is (though, we do
 	 * know how many NUL-terminated strings are in the vector).
 	 */
 	len = 0;
 	count = pss.ps_nargvstr;
-	while (count && len < ARG_MAX) {
-		aiov.iov_base = &arg[len];
-		aiov.iov_len = 1;
+	upper_bound = round_page(uio->uio_offset+1);
+	for (; count && len < upper_bound; len += PAGE_SIZE) {
+		aiov.iov_base = arg;
+		aiov.iov_len = PAGE_SIZE;
 		auio.uio_iov = &aiov;
 		auio.uio_iovcnt = 1;
 		auio.uio_offset = argv + len;
-		auio.uio_resid = 1;
+		auio.uio_resid = PAGE_SIZE;
 		auio.uio_segflg = UIO_SYSSPACE;
 		auio.uio_rw = UIO_READ;
 		auio.uio_procp = NULL;
@@ -176,9 +175,16 @@
 		if (error)
 			goto bad;
 
-		if (len > 0 && arg[len] == '\0')
-			count--;	/* one full string */
-		len++;
+		for(i=len; i < (len + PAGE_SIZE) && count; i++) {
+			if (arg[i] == '\0')
+				count--;	/* one full string */
+		}
+
+		if (!count) {
+			/* no more argv strings, set up len and break */
+			len = i;
+			break;
+		}
 	}
 	if (len > 0)
 		len--;			/* exclude last NUL */
@@ -186,30 +192,24 @@
 	/*
 	 * Release the process.
 	 */
-#if defined(UVM)
 	PRELE(p);
+#if defined(UVM)
 	uvmspace_free(p->p_vmspace);
-#else
-	PRELE(p);
 #endif
 
  doio:
 	xlen = len - uio->uio_offset;
-	xlen = imin(xlen, uio->uio_resid);
 	if (xlen <= 0) 
 		error = 0;
 	else
-		error = uiomove(arg + uio->uio_offset, xlen, uio);
-
+		error = uiomove(arg + trunc_page(len), xlen, uio);
 	free(arg, M_TEMP);
 	return (error);
 
  bad:
-#if defined(UVM)
 	PRELE(p);
+#if defined(UVM)
 	uvmspace_free(p->p_vmspace);
-#else
-	PRELE(p);
 #endif
 	free(arg, M_TEMP);
 	return (error);
>Audit-Trail:
>Unformatted: