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

Johannes Sixt [off-list ref] writes:

Paolo Bonzini schrieb:

quoted

+	/*
+	 * 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?

Junio C Hamano wrote:

Johannes Sixt [off-list ref] writes:

quoted

Paolo Bonzini schrieb:

quoted

+	/*
+	 * 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.

Paolo

Paolo Bonzini schrieb:

Junio C Hamano wrote:

quoted

Johannes Sixt [off-list ref] writes:

quoted

Paolo Bonzini schrieb:

quoted

+	/*
+	 * 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 straced tee (it was the only tool I found that opens files for writing
without also opening some for reading). If one of 0,1,2 is closed, it
*does* dup() the fd that it is going to write.

Don't you now feel like Reg in "Life of Brian":

"All right, but apart from the sanitation, the medicine, education, wine,
public order, irrigation, roads, a fresh water system, and public health,
what have the Romans ever done for us?"

;)

-- Hannes

quoted

Busybox.  But it runs setuid, as Steven pointed out.

I straced tee (it was the only tool I found that opens files for writing
without also opening some for reading). If one of 0,1,2 is closed, it
*does* dup() the fd that it is going to write.

To be precise, it does a blind "dup2 (fd, 3)" and goes on with file
descriptor 3.

open("foo", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0666) = 0
fcntl64(0, F_DUPFD, 3)                  = 3
close(0)                                = 0

Paolo

On Tue, Aug 26, 2008 at 10:38:35AM -0700, Junio C Hamano wrote:

This is going too far.  Have you seen any other sane program that do this?

Hmm. I posted a patch to the "sane" project (scanner daemon) to avoid a
similar problem. The patch was rejected.

saned tries to sanitise its environment, specifically low order fds:

fd = open("/dev/null", O_RDWR);
dup2(fd, 0);
dup2(fd, 1);
fup2(fd, 2);
close(fd);

And I pointed out that if the fds aren't sanitary (all fds open) before
the code snippet, they won't be sanitary after it. My patch was only:

if (fd > 2)
    close(fd);

If closing fd 0/1/2 and then forking a subprocess is the unix equivalent
of delayed shooting yourself in the foot then I can agree; git doesn't
need it.

Nick.