From owner-freebsd-hackers@FreeBSD.ORG Sat Nov 5 13:58:05 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C84CC1065673; Sat, 5 Nov 2011 13:58:05 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 64B1C8FC0A; Sat, 5 Nov 2011 13:58:04 +0000 (UTC) Received: from alf.home (alf.kiev.zoral.com.ua [10.1.1.177]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id pA5Dw1YJ011620 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 5 Nov 2011 15:58:01 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: from alf.home (kostik@localhost [127.0.0.1]) by alf.home (8.14.5/8.14.5) with ESMTP id pA5Dw1Kn050911; Sat, 5 Nov 2011 15:58:01 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by alf.home (8.14.5/8.14.5/Submit) id pA5Dw1Si050910; Sat, 5 Nov 2011 15:58:01 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: alf.home: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 5 Nov 2011 15:58:01 +0200 From: Kostik Belousov To: Mikolaj Golub Message-ID: <20111105135801.GT50300@deviant.kiev.zoral.com.ua> References: <86y5wkeuw9.fsf@kopusha.home.net> <20111016171005.GB50300@deviant.kiev.zoral.com.ua> <86aa8qozyx.fsf@kopusha.home.net> <20111025082451.GO50300@deviant.kiev.zoral.com.ua> <86aa8k2im0.fsf@kopusha.home.net> <20111031094948.GB50300@deviant.kiev.zoral.com.ua> <86vcr21agm.fsf@kopusha.home.net> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="KdGTaGOs1nMop4we" Content-Disposition: inline In-Reply-To: <86vcr21agm.fsf@kopusha.home.net> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.9 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-hackers@freebsd.org, Robert Watson Subject: Re: "ps -e" without procfs(5) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Nov 2011 13:58:06 -0000 --KdGTaGOs1nMop4we Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. --KdGTaGOs1nMop4we Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.18 (FreeBSD) iEYEARECAAYFAk61QOkACgkQC3+MBN1Mb4iLrQCfSx0FaNO0VUK/tqaGtGCvedNR A94An3n7F1WOAjbrTIqT6wQ2cbcG8Paa =F5gk -----END PGP SIGNATURE----- --KdGTaGOs1nMop4we--