From owner-freebsd-hackers Thu Jun 20 0: 4:37 2002 Delivered-To: freebsd-hackers@freebsd.org Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by hub.freebsd.org (Postfix) with ESMTP id 5162737B401; Thu, 20 Jun 2002 00:04:29 -0700 (PDT) Received: from mousie.catspoiler.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.12.3/8.12.3) with ESMTP id g5K74OM1068558; Thu, 20 Jun 2002 00:04:28 -0700 (PDT) (envelope-from dl-freebsd@catspoiler.org) Message-Id: <200206200704.g5K74OM1068558@gw.catspoiler.org> Date: Thu, 20 Jun 2002 00:04:41 -0700 (PDT) From: Don Lewis Subject: fdcheckstd() test bug in execve() (was: Re: Suggested fixes for uidinfo "would sleep" messages) To: makonnen@pacbell.net Cc: bright@mu.org, jhb@FreeBSD.ORG, hackers@FreeBSD.ORG In-Reply-To: <20020619191846.4aa74dd6.makonnen@pacbell.net> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=US-ASCII Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On 19 Jun, Mike Makonnen wrote: > I actually submitted a patch for this a few days ago to John Baldwin, but > I think he's busy right now 'cause I haven't heard from him. Here it > is: Your patch also looks like it should fix the bug. I prefer my patch, though, because I think the resultant code is structured better and should be easier to understand. For instance, the reason for the assignment to oldcred in the "if (error != 0)" block in your patch is not immediately obvious. My version should probably run at tiny bit faster as well, though this is unlikely to be measureable. BTW, it is possible to further optimize the code if both instances of: p->p_ucred = newcred; newcred = NULL; are changed to: p->p_ucred = newcred; newcred = oldcred; This allows: if (newcred == NULL) crfree(oldcred); else crfree(newcred); to be simplified to: crfree(newcred); and the initialization of newcred in its declaration could be eliminated. I'd rename newcred to something else like cred or tmpcred that would hopefully be less likely to cause confusion. > Index: sys/kern/kern_exec.c > =================================================================== > RCS file: /home/ncvs/src/sys/kern/kern_exec.c,v > retrieving revision 1.164 > diff -u -r1.164 kern_exec.c > --- sys/kern/kern_exec.c 7 Jun 2002 05:41:27 -0000 1.164 > +++ sys/kern/kern_exec.c 16 Jun 2002 14:14:37 -0000 > @@ -133,7 +133,7 @@ > struct image_params image_params, *imgp; > struct vattr attr; > int (*img_first)(struct image_params *); > - struct pargs *oldargs, *newargs = NULL; > + struct pargs *oldargs=NULL, *newargs = NULL; > struct procsig *oldprocsig, *newprocsig; > #ifdef KTRACE > struct vnode *tracevp = NULL; > @@ -383,8 +383,10 @@ > #endif > /* Make sure file descriptors 0..2 are in use. */ > error = fdcheckstd(td); > - if (error != 0) > - goto exec_fail_dealloc; > + if (error != 0) { > + oldcred = NULL; > + goto done1; > + } > /* > * Set the new credentials. > */ > @@ -467,6 +469,7 @@ > p->p_args = newargs; > newargs = NULL; > } > +done1: > PROC_UNLOCK(p); > > /* > @@ -476,7 +479,6 @@ > crfree(oldcred); > else > crfree(newcred); > - KASSERT(newargs == NULL, ("leaking p_args")); > /* > * Handle deferred decrement of ref counts. > */ > @@ -486,7 +488,10 @@ > if (tracevp != NULL) > vrele(tracevp); > #endif > - pargs_drop(oldargs); > + if (oldargs != NULL) > + pargs_drop(oldargs); > + if (newargs != NULL) > + pargs_drop(newargs); > > exec_fail_dealloc: > To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message