On 11 Sep, John Baldwin wrote:
>
> On 11-Sep-2002 Don Lewis wrote:
>> On 10 Sep, Don Lewis wrote:
>>> On 10 Sep, Nate Lawson wrote:
>>>
>>>> I'm not sure why fdcheckstd() and setugidsafety() couldn't both happen
>>>> before grabbing the proc lock. Dropping locks in the middle or
>>>> pre-allocating should always be a last resort.
>>>
>>> That is ok as long as there aren't other threads that can mess things up
>>> after fdcheckstd() and setugidsafety() have done their thing.
>>
>> It looks like threads aren't a problem because of the call to
>> thread_single() at the top of execve(). Ptrace() is still a potential
>> problem, so we can't call fdcheckstd() and setugidsafety() until after
>> credential_changing has been calculated, setsugid() has been called and
>> tracing has been disabled, all of which are done after proc lock has
>> been grabbed.
>>
>> It also looks like we should pre-allocate newprocsig instead of
>> temporarily dropping the lock.
>
> We used to do that but backed it out because it wasn't deemed to be
> that necessary. If you have a demonstrable problematic race condition
> that this fixes then it might be a good idea. However, allocating
> stuff we don't need isn't but so great either.
I haven't observed any problems with the procsig stuff, but it sure hit
me in the eye when I was scanning the code to see if the fdcheckfd()
could be done before grabbing the proc lock. The mp_fixme() suggests to
me that the entire if block is going to be protected by its own lock
sometime in the future:
PROC_LOCK(p);
mp_fixme("procsig needs a lock");
if (p->p_procsig->ps_refcnt > 1) {
oldprocsig = p->p_procsig;
PROC_UNLOCK(p);
MALLOC(newprocsig, struct procsig *, sizeof(struct procsig),
M_SUBPROC, M_WAITOK);
bcopy(oldprocsig, newprocsig, sizeof(*newprocsig));
newprocsig->ps_refcnt = 1;
oldprocsig->ps_refcnt--;
PROC_LOCK(p);
p->p_procsig = newprocsig;
if (p->p_sigacts == &p->p_uarea->u_sigacts)
panic("shared procsig but private sigacts?");
p->p_uarea->u_sigacts = *p->p_sigacts;
p->p_sigacts = &p->p_uarea->u_sigacts;
}
We probably won't want to hold the lock across the call to MALLOC(), and
dropping the lock and possibly blocking, giving ps_refcnt the
opportunity to change behind our back, doesn't seem wise. Copying a
shared object and manipulating its reference count while it is unlocked
doesn't sound wise either. We may be protected in other ways at the
moment, but this code suggests to me that this won't always be the case.
> I think ptrace(2)
> is not an issue because ptrace(2) won't attach to a P_INEXEC process
> IIRC.
I think you're correct, but we still can't call fdcheckstd() before we
know that we are doing the set-id case, and that decision is made after
we've grabbed the proc lock. Do you think it is reasonable to
temporarily drop the proc lock for the fdcheckstd() call?
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
