From: Karl Chen <hidden> Date: 2016-06-15 22:45:13
I ran into what I think is a bug:
sh$ git fetch 0<&-
(i.e. run git-fetch with stdin closed.)
It aborts with:
fatal: read error (Bad file descriptor)
I think the problem arises from the use of dup2+close in
start_command(). It wants to rename a pipe file descriptor to 0,
so it does
dup2(from, to);
close(from);
... but in this case from == to == 0, so
dup2(0, 0);
close(0);
just ends up closing the pipe.
The patch below fixes the problem for me.
From 78446c82131a5ca7f22f92bc32d7f3036bba9629 Mon Sep 17 00:00:00 2001
From: Karl Chen <redacted>
Date: Mon, 25 Aug 2008 01:09:08 -0700
Subject: [PATCH] Fix start_command() pipe bug when stdin is closed.
When intending to rename a fd to 0, if the fd is already 0, then do nothing,
instead of dup2(0,0); close(0);
The problematic behavior could be seen thus: git-fetch 0<&-
Signed-off-by: Karl Chen <redacted>
---
run-command.c | 29 +++++++++++++++++------------
1 files changed, 17 insertions(+), 12 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:45:13
Karl Chen schrieb:
I ran into what I think is a bug:
sh$ git fetch 0<&-
(i.e. run git-fetch with stdin closed.)
It aborts with:
fatal: read error (Bad file descriptor)
When I try these instructions I don't get an error; instead the command
runs successfully.
I think the problem arises from the use of dup2+close in
start_command(). It wants to rename a pipe file descriptor to 0,
so it does
dup2(from, to);
close(from);
... but in this case from == to == 0, so
dup2(0, 0);
close(0);
just ends up closing the pipe.
While I do see that there is a problem, it is only half of the story, and
your patch addresses only this half.
What if stdout is closed, too? Then the ends of the first allocated pipe
would go to fds 0 and 1, and then the pipe end at 1 would be closed by a
subsequent dup2(xxx, 1).
Junio, what's your take on this?
-- Hannes
From: Paolo Bonzini <hidden> Date: 2016-06-15 22:45:13
While I do see that there is a problem, it is only half of the story, and
your patch addresses only this half.
What if stdout is closed, too? Then the ends of the first allocated pipe
would go to fds 0 and 1, and then the pipe end at 1 would be closed by a
subsequent dup2(xxx, 1).
What about opening files (in start_command, protected by a loop that run
only once, or on startup) until you get a descriptor that is > 2? Like
this:
static int low_fds_reserved;
if (!low_fds_reserved)
{
int fd = open("/dev/null", O_RDWR);
while (fd >= 0 && fd <= 2)
fd = dup (fd);
if (fd != -1)
close (fd);
else
perror ("start_command");
low_fds_reserved = 1;
}
Paolo
From: Paolo Bonzini <hidden> Date: 2016-06-15 22:45:13
There is a problem in the use of dup2+close in start_command()
when one or more of file descriptors 0/1/2 are closed. In order
to rename a pipe file descriptor to 0, it does
dup2(from, 0);
close(from);
... but if stdin was closed (for example) from == 0, so that
dup2(0, 0);
close(0);
just ends up closing the pipe. This patch fixes it by opening all of
the "low" descriptors to /dev/null.
In most cases this patch will not cause any additional system calls;
actually by reusing the /dev/null descriptor when possible (instead
of opening a fresh one in dup_devnull) it may even save a handful in
some cases. :-)
Signed-off-by: Paolo Bonzini <redacted>
---
run-command.c | 35 ++++++++++++++++++++++-------------
1 files changed, 22 insertions(+), 13 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:45:13
Paolo Bonzini schrieb:
There is a problem in the use of dup2+close in start_command()
when one or more of file descriptors 0/1/2 are closed.
"Karl Chen pointed out a problem..." (just to give due credit).
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, open 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));
+ }
Except for the insane GNU style indentation ;-) this makes a lot of sense.
Acked-by: Johannes Sixt <redacted>
The changes to the MINGW32 section are good (they pass the test suite).
Thanks for taking care of that.
-- Hannes
From: Paolo Bonzini <hidden> Date: 2016-06-15 22:45:13
Karl Chen pointed out a problem in the use of dup2+close in
start_command() when one or more of file descriptors 0/1/2 are closed.
In order to rename a pipe file descriptor to 0, it does
dup2(from, 0);
close(from);
... but if stdin was closed (for example) from == 0, so that
dup2(0, 0);
close(0);
just ends up closing the pipe. This patch fixes it by opening all of
the "low" descriptors to /dev/null.
In most cases this patch will not cause any additional system calls;
actually by reusing the /dev/null descriptor when possible (instead
of opening a fresh one in dup_devnull) it may even save a handful in
some cases. :-)
Signed-off-by: Paolo Bonzini <redacted>
Acknowledged-by: Johannes Sixt [off-list ref]
---
run-command.c | 35 ++++++++++++++++++++++-------------
1 files changed, 22 insertions(+), 13 deletions(-)
> "Karl Chen pointed out a problem..." (just to give due credit).
Of course.
> Except for the insane GNU style indentation ;-)
*blush* -- both problems deriving from too hasty e-mail cut&paste.
> this makes a lot of sense. [...] MINGW32 [...] pass the test suite.
Thanks, also for testing Windows.
Paolo