"ps -e" without procfs(5)

[Kostik Belousov]

On Wed, Nov 02, 2011 at 11:27:37PM +0200, Mikolaj Golub wrote:
>
> On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote:
>
>  KB> I think it is better to use sys/elf.h over the machine/elf.h.
>
>  KB> Please change the comment for PROC_AUXV_MAX to "Safety limit on
>  KB> auxv size". Also, it worth adding a comment saying that we are
>  KB> reading aux vectors twice, first to get a size, second time to
>  KB> fetch a content, for simplicity.
>
>  KB> When reading aux vector, if the PROC_AUXV_MAX entries are
>  KB> iterated over, and we still not reached AT_NULL, the return error
>  KB> is 0. Was it intended ?
>
>  KB> For PROC_ARG and PROC_ENV, you blindly trust the read values of
>  KB> the arg and env vector sizes. This can easily cause kernel panics
>  KB> due to unability to malloc the requested memory. I recommend to
>  KB> put some clump, and twice of (PATH_MAX + ARG_MAX) is probably
>  KB> enough (see kern_exec.c, in particular, exec_alloc_args). Also,
>  KB> you might use the swappable memory for the strings as well, in
>  KB> the style of exec_alloc_args().
>
>  KB> I suspect this is my bug: Reading the GET_PS_STRINGS_CHUNK_SZ may
>  KB> validly return EFAULT if the string is shorter than the chunk
>  KB> and aligned at the end of the page, assuming the next page is
>  KB> not mapped. There should be a fallback to fubyte() read loop. I
>  KB> remember that copyinstr() was unsuitable.
>
>  KB> The checks for P_WEXIT in the linprocfs routines look strange.
>  KB> Since you are unlocking the process right after the check, it
>  KB> does not make sense. In fact, the checks are not needed, I
>  KB> believe, since pseudofs already did the hold (see e.g. pfs_read
>  KB> and pfs_visible).
>
> Here is an updated version of the patch. Also available at
>
> http://people.freebsd.org/~trociny/env.sys.1.patch
>
> I decided to use the same constant (PROC_VECTOR_MAX) for limiting both
> the number of arg or env strings and the numbex of aux vectors.
>
> Also I decided not to play with exec_alloc_args :-).
+	if (error == EFAULT) {
+		for (i = 0; i < len; i++) {
+			c = fubyte(sptr + i);
+			if (c < 0)
As a purely stylistical issue, compare with -1.

+				return (EFAULT);
+			buf[i] = (char)c;
+			if (c == '\0')
+				break;
+		}
+		error = 0;
+	}
+	return error;
Put () around error.

+	/*
+	 * Check that that the address is in user space.
+	 */
+	if (vptr + 1 < VM_MIN_ADDRESS + 1 || vptr >= VM_MAXUSER_ADDRESS)
+		return (ENOEXEC);
Why is this needed ?

I think that the aux vector must be naturally aligned. You can return
ENOEXEC early if vptr is not aligned.

Why the blank after the loop statement in get_ps_strings() ?

There shall be blank lines after the '{' in proc_getargv() and proc_getenvv().

Note that using cached pargs is somewhat inconsistent with the digging
into ps_strings.

procfs_doproccmdline() can benefit from your work.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20111105/0db6b1b3/attachment.pgp