Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2008, 06:09 UTC
Message-ID: <7vbpzgb94q.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <E1KXcH3-0000zJ-0m@fencepost.gnu.org>

Paolo Bonzini <bonzini@gnu.org> writes:

Show 18 quoted lines

>  int start_command(struct child_process *cmd)
>  {
>  	int need_in, need_out, need_err;
>  	int fdin[2], fdout[2], fderr[2];
>  
>  	/*
> +	 * Make sure that all file descriptors <= 2 are open, otherwise we
> +	 * mess them up when dup'ing pipes onto stdin/stdout/stderr.  Since
> +	 * we are at it, save a file descriptor on /dev/null to use it later.
> +	 */
> +	if (devnull_fd == -1) {
> +		devnull_fd = open("/dev/null", O_RDWR);
> +		while (devnull_fd >= 0 && devnull_fd <= 2)
> +			devnull_fd = dup(devnull_fd);
> +		if (devnull_fd == -1)
> +			die("opening /dev/null failed (%s)", strerror(errno));
> +	}
> +

I may be misreading the patch but, this logic always opens /dev/null, if nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and worse, it keeps fd=3 open.

Making sure low fds 0, 1 and 2 are open is a good thing. I do not think clobbering fd=3 is good.

Also shouldn't this be done only on the side that dup()s fds around, i.e. in the child process after fork()? Why is this done for the parent?