Re: [PATCH] be paranoid about closed stdin/stdout/stderr

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2008, 23:04 UTC
Message-ID: <7v3akr2xa3.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vabez2yac.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 22 quoted lines

> Paolo Bonzini <bonzini@gnu.org> writes:
>
>> Junio C Hamano wrote:
>>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>> 
>>>> Paolo Bonzini schrieb:
>>>>> +	/*
>>>>> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
>>>>> +	 * in die().  It also avoids not messing up when the pipes are
>>>>> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
>>>>> +	 */
>>>> I see your point, but I don't have an opinion whether this stretch is
>>>> necessary.
>>> 
>>> This is going too far.  Have you seen any other sane program that do this?
>>
>> Busybox.  But it runs setuid, as Steven pointed out.
>>
>> I say it's all (i.e. be this paranoid), or nothing.
>
> I tend to agree, and I think what Stephen R. van den Berg said earlier in
> the thread makes perfect sense.

So going back to the very original in the thread.

I think

	$ git fetch 0<&-

from the command line is a mere user stupidity.

On the other hand, if a cron/at job that contains "git fetch" is launched in an environment with fd#0 (or #1 or #2 for that matter) closed, it would certainly be problematic. It can easily be worked around by redirecting file descriptors appropriately in the script that is launched, though.

On a related note, we should make sure that we run our hooks with the set of low file descriptors opened sensibly. It would be a bug if we are running them in a weird environment and forcing them to do funky redirection themselves. I think we are already Ok in this regard, but I didn't check.