Date: Thu, 20 Jun 2002 00:04:41 -0700 (PDT) From: Don Lewis <dl-freebsd@catspoiler.org> To: makonnen@pacbell.net Cc: bright@mu.org, jhb@FreeBSD.ORG, hackers@FreeBSD.ORG Subject: fdcheckstd() test bug in execve() (was: Re: Suggested fixes for uidinfo "would sleep" messages) Message-ID: <200206200704.g5K74OM1068558@gw.catspoiler.org> In-Reply-To: <20020619191846.4aa74dd6.makonnen@pacbell.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200206200704.g5K74OM1068558>