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.
pgpBc5VnV0yxI.pgp
Description: PGP signature
