Date: Wed, 19 Jun 2002 15:56:12 -0700 (PDT) From: Don Lewis <dl-freebsd@catspoiler.org> To: bright@mu.org Cc: jhb@freebsd.org, hackers@freebsd.org Subject: Re: Suggested fixes for uidinfo "would sleep" messages Message-ID: <200206192256.g5JMuCM1067585@gw.catspoiler.org> In-Reply-To: <20020619172107.GF85935@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 19 Jun, Alfred Perlstein wrote:
> re execve() issues...
>
> * Don Lewis <dl-freebsd@catspoiler.org> [020619 01:10] wrote:
>> On 18 Jun, Alfred Perlstein wrote:
>> > Thanks a ton for taking care of it, your patch is actually cleaner
>> > than what I had started on, I'll be committing it shortly.
>>
>> While you're working in this area, take a look at execve(). If the
>> fdcheckstd() test fails, we leak ucred and uidinfo structures, and also
>> leave the proc locked. The fix is pretty straightforward.
>
> I'm a bit confused actually, it looks like just unlocking the proc
> and then moving the 'exec_fail_dealloc' label higher would fix it,
> except I'm not sure about the:
I was thinking more along the lines of how this is handled in the
set*uid() implementations, basically calling PROC_UNLOCK() and doing the
deallocation within the "if" block before bailing out. The problem with
sliding the label is that you have to also initialize euip to NULL at
the beginning and then check its value before calling uifree() so that
you don't pass garbage to uifree().
> /*
> * Handle deferred decrement of ref counts.
> */
> if (textvp != NULL)
> vrele(textvp);
> #ifdef KTRACE
> if (tracevp != NULL)
> vrele(tracevp);
> #endif
> pargs_drop(oldargs);
>
> part... should that be before or after exec_fail_dealloc?
I hadn't looked at this before ...
I think the proper solution is to move the fdcheckstd() test to the
beginning of its enclosing "if" block so that textvp and tracevp are
only set after the test is passed. I'd rearrange the code so that all
the cleanup at the end is done in the reverse order that the allocations
were done at the beginning. Doing so allowed me to see that there is
also a potential leak of newargs. The following patch compiles without
error, but I haven't had a chance to run it.
--- kern_exec.c.prev Tue Jun 18 12:09:06 2002
+++ kern_exec.c Wed Jun 19 15:26:09 2002
@@ -368,6 +368,12 @@
((attr.va_mode & VSGID) && oldcred->cr_gid != attr.va_gid)) &&
(imgp->vp->v_mount->mnt_flag & MNT_NOSUID) == 0 &&
(p->p_flag & P_TRACED) == 0) {
+ /* Make sure file descriptors 0..2 are in use. */
+ error = fdcheckstd(td);
+ if (error != 0) {
+ PROC_UNLOCK(p);
+ goto exec_fail_dealloc_more;
+ }
/*
* Turn off syscall tracing for set-id programs, except for
* root. Record any set-id flags first to make sure that
@@ -383,10 +389,6 @@
mtx_unlock(&ktrace_mtx);
}
#endif
- /* Make sure file descriptors 0..2 are in use. */
- error = fdcheckstd(td);
- if (error != 0)
- goto exec_fail_dealloc;
/*
* Set the new credentials.
*/
@@ -471,15 +473,9 @@
}
PROC_UNLOCK(p);
- /*
- * Free any resources malloc'd earlier that we didn't use.
- */
- uifree(euip);
- if (newcred == NULL)
- crfree(oldcred);
- else
- crfree(newcred);
KASSERT(newargs == NULL, ("leaking p_args"));
+ pargs_drop(oldargs);
+
/*
* Handle deferred decrement of ref counts.
*/
@@ -489,7 +485,18 @@
if (tracevp != NULL)
vrele(tracevp);
#endif
- pargs_drop(oldargs);
+
+exec_fail_dealloc_more:
+ /*
+ * Free any resources malloc'd earlier that we didn't use.
+ */
+ if (newargs != NULL)
+ pargs_drop(newargs);
+ uifree(euip);
+ if (newcred == NULL)
+ crfree(oldcred);
+ else
+ crfree(newcred);
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?200206192256.g5JMuCM1067585>