{"thread":{"id":"15195","subject":"[PATCH] Fix start_command() pipe bug when stdin is closed.","startedAt":"2008-08-25T08:28:19Z","lastAt":"2008-08-28T13:58:46Z","messageCount":37,"participants":["Karl Chen","Johannes Sixt","Paolo Bonzini","Junio C Hamano","Stephen R. van den Berg","Avery Pennarun","Nick Andrew"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"88437","messageId":"quack.20080825T0128.lthr68djy70@roar.cs.berkeley.edu","threadId":"15195","inReplyTo":null,"subject":"[PATCH] Fix start_command() pipe bug when stdin is closed.","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-25T08:28:19Z","receivedAt":"2008-08-25T08:28:19Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":"\nI ran into what I think is a bug:\n sh$ git fetch 0<&-\n\n(i.e. run git-fetch with stdin closed.)\nIt aborts with:\n fatal: read error (Bad file descriptor)\n\nI think the problem arises from the use of dup2+close in\nstart_command(). It wants to rename a pipe file descriptor to 0,\nso it does\n dup2(from, to);\n close(from);\n\n... but in this case from == to == 0, so \n dup2(0, 0);\n close(0);\njust ends up closing the pipe.\n\nThe patch below fixes the problem for me.\n\n\n>From 78446c82131a5ca7f22f92bc32d7f3036bba9629 Mon Sep 17 00:00:00 2001\nFrom: Karl Chen \nDate: Mon, 25 Aug 2008 01:09:08 -0700\nSubject: [PATCH] Fix start_command() pipe bug when stdin is closed.\n\nWhen intending to rename a fd to 0, if the fd is already 0, then do nothing,\ninstead of dup2(0,0); close(0);\n\nThe problematic behavior could be seen thus: git-fetch 0<&-\n\nSigned-off-by: Karl Chen \n\n---\n run-command.c | 29 +++++++++++++++++------------\n 1 files changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex caab374..b4bd80f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -8,11 +8,18 @@ static inline void close_pair(int fd[2])\n \tclose(fd[1]);\n }\n \n+static inline void rename_fd(int from, int to)\n+{\n+\tif (from != to) {\n+\t\tdup2(from, to);\n+\t\tclose(from);\n+\t}\n+}\n+\n static inline void dup_devnull(int to)\n {\n \tint fd = open(\"/dev/null\", O_RDWR);\n-\tdup2(fd, to);\n-\tclose(fd);\n+\trename_fd(fd, to);\n }\n \n int start_command(struct child_process *cmd)\n@@ -74,18 +81,17 @@ int start_command(struct child_process *cmd)\n \t\tif (cmd->no_stdin)\n \t\t\tdup_devnull(0);\n \t\telse if (need_in) {\n-\t\t\tdup2(fdin[0], 0);\n-\t\t\tclose_pair(fdin);\n+\t\t\trename_fd(fdin[0], 0);\n+\t\t\tclose(fdin[1]);\n \t\t} else if (cmd->in) {\n-\t\t\tdup2(cmd->in, 0);\n-\t\t\tclose(cmd->in);\n+\t\t\trename_fd(cmd->in, 0);\n \t\t}\n \n \t\tif (cmd->no_stderr)\n \t\t\tdup_devnull(2);\n \t\telse if (need_err) {\n-\t\t\tdup2(fderr[1], 2);\n-\t\t\tclose_pair(fderr);\n+\t\t\trename_fd(fderr[1], 2);\n+\t\t\tclose(fderr[0]);\n \t\t}\n \n \t\tif (cmd->no_stdout)\n@@ -93,11 +99,10 @@ int start_command(struct child_process *cmd)\n \t\telse if (cmd->stdout_to_stderr)\n \t\t\tdup2(2, 1);\n \t\telse if (need_out) {\n-\t\t\tdup2(fdout[1], 1);\n-\t\t\tclose_pair(fdout);\n+\t\t\trename_fd(fdout[1], 1);\n+\t\t\tclose(fdout[0]);\n \t\t} else if (cmd->out > 1) {\n-\t\t\tdup2(cmd->out, 1);\n-\t\t\tclose(cmd->out);\n+\t\t\trename_fd(cmd->out, 1);\n \t\t}\n \n \t\tif (cmd->dir && chdir(cmd->dir))\n-- \n1.5.6.2\n"},{"id":"88442","messageId":"48B28CF8.2060306@viscovery.net","threadId":"15195","inReplyTo":"quack.20080825T0128.lthr68djy70@roar.cs.berkeley.edu","subject":"Re: [PATCH] Fix start_command() pipe bug when stdin is closed.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-25T10:44:08Z","receivedAt":"2008-08-25T10:44:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Karl Chen schrieb:\n> I ran into what I think is a bug:\n> sh$ git fetch 0<&-\n> \n> (i.e. run git-fetch with stdin closed.)\n> It aborts with:\n> fatal: read error (Bad file descriptor)\n\nWhen I try these instructions I don't get an error; instead the command\nruns successfully.\n\n> I think the problem arises from the use of dup2+close in\n> start_command(). It wants to rename a pipe file descriptor to 0,\n> so it does\n> dup2(from, to);\n> close(from);\n> \n> ... but in this case from == to == 0, so \n> dup2(0, 0);\n> close(0);\n> just ends up closing the pipe.\n\nWhile I do see that there is a problem, it is only half of the story, and\nyour patch addresses only this half.\n\nWhat if stdout is closed, too? Then the ends of the first allocated pipe\nwould go to fds 0 and 1, and then the pipe end at 1 would be closed by a\nsubsequent dup2(xxx, 1).\n\nJunio, what's your take on this?\n\n-- Hannes\n"},{"id":"88445","messageId":"48B29C52.8040901@gnu.org","threadId":"15195","inReplyTo":"48B28CF8.2060306@viscovery.net","subject":"Re: [PATCH] Fix start_command() pipe bug when stdin is closed.","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-25T11:49:38Z","receivedAt":"2008-08-25T11:49:38Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"\n> While I do see that there is a problem, it is only half of the story, and\n> your patch addresses only this half.\n> \n> What if stdout is closed, too? Then the ends of the first allocated pipe\n> would go to fds 0 and 1, and then the pipe end at 1 would be closed by a\n> subsequent dup2(xxx, 1).\n\nWhat about opening files (in start_command, protected by a loop that run\nonly once, or on startup) until you get a descriptor that is > 2? Like\nthis:\n\n static int low_fds_reserved;\n if (!low_fds_reserved)\n {\n int fd = open(\"/dev/null\", O_RDWR);\n while (fd >= 0 && fd <= 2)\n fd = dup (fd);\n if (fd != -1)\n close (fd);\n else\n perror (\"start_command\");\n low_fds_reserved = 1;\n }\n\nPaolo\n"},{"id":"88446","messageId":"E1KXawS-0001gg-Ty@fencepost.gnu.org","threadId":"15195","inReplyTo":"48B29C52.8040901@gnu.org","subject":"[PATCH v2] fix start_command() bug when stdin is closed","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-25T12:00:35Z","receivedAt":"2008-08-25T12:00:35Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"There is a problem in the use of dup2+close in start_command()\nwhen one or more of file descriptors 0/1/2 are closed. In order\nto rename a pipe file descriptor to 0, it does\n\n dup2(from, 0);\n close(from);\n\n... but if stdin was closed (for example) from == 0, so that\n\n dup2(0, 0);\n close(0);\n\njust ends up closing the pipe. This patch fixes it by opening all of\nthe \"low\" descriptors to /dev/null.\n\nIn most cases this patch will not cause any additional system calls;\nactually by reusing the /dev/null descriptor when possible (instead\nof opening a fresh one in dup_devnull) it may even save a handful in\nsome cases. :-)\n\nSigned-off-by: Paolo Bonzini \n---\n run-command.c | 35 ++++++++++++++++++++++-------------\n 1 files changed, 22 insertions(+), 13 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex caab374..4619494 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -2,25 +2,34 @@\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n \n+static int devnull_fd = -1;\n+\n static inline void close_pair(int fd[2])\n {\n \tclose(fd[0]);\n \tclose(fd[1]);\n }\n \n-static inline void dup_devnull(int to)\n-{\n-\tint fd = open(\"/dev/null\", O_RDWR);\n-\tdup2(fd, to);\n-\tclose(fd);\n-}\n-\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n \tint fdin[2], fdout[2], fderr[2];\n \n \t/*\n+\t * Make sure that all file descriptors <= 2 are open, otherwise we\n+\t * mess them up when dup'ing pipes onto stdin/stdout/stderr. Since\n+\t * we are at it, open a file descriptor on /dev/null to use it later.\n+\t */\n+\tif (devnull_fd == -1)\n+\t {\n+\t devnull_fd = open(\"/dev/null\", O_RDWR);\n+\t while (devnull_fd >= 0 && devnull_fd <= 2)\n+\t devnull_fd = dup(devnull_fd);\n+\t if (devnull_fd == -1)\n+\t die(\"opening /dev/null failed (%s)\", strerror(errno));\n+\t }\n+\n+\t/*\n \t * In case of errors we must keep the promise to close FDs\n \t * that have been passed in via ->in and ->out.\n \t */\n@@ -72,7 +81,7 @@ int start_command(struct child_process *cmd)\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n \t\tif (cmd->no_stdin)\n-\t\t\tdup_devnull(0);\n+\t\t\tdup2(devnull_fd, 0);\n \t\telse if (need_in) {\n \t\t\tdup2(fdin[0], 0);\n \t\t\tclose_pair(fdin);\n@@ -82,14 +91,14 @@ int start_command(struct child_process *cmd)\n \t\t}\n \n \t\tif (cmd->no_stderr)\n-\t\t\tdup_devnull(2);\n+\t\t\tdup2(devnull_fd, 2);\n \t\telse if (need_err) {\n \t\t\tdup2(fderr[1], 2);\n \t\t\tclose_pair(fderr);\n \t\t}\n \n \t\tif (cmd->no_stdout)\n-\t\t\tdup_devnull(1);\n+\t\t\tdup2(devnull_fd, 1);\n \t\telse if (cmd->stdout_to_stderr)\n \t\t\tdup2(2, 1);\n \t\telse if (need_out) {\n@@ -127,7 +136,7 @@ int start_command(struct child_process *cmd)\n \n \tif (cmd->no_stdin) {\n \t\ts0 = dup(0);\n-\t\tdup_devnull(0);\n+\t\tdup2(devnull_fd, 0);\n \t} else if (need_in) {\n \t\ts0 = dup(0);\n \t\tdup2(fdin[0], 0);\n@@ -138,7 +147,7 @@ int start_command(struct child_process *cmd)\n \n \tif (cmd->no_stderr) {\n \t\ts2 = dup(2);\n-\t\tdup_devnull(2);\n+\t\tdup2(devnull_fd, 2);\n \t} else if (need_err) {\n \t\ts2 = dup(2);\n \t\tdup2(fderr[1], 2);\n@@ -146,7 +155,7 @@ int start_command(struct child_process *cmd)\n \n \tif (cmd->no_stdout) {\n \t\ts1 = dup(1);\n-\t\tdup_devnull(1);\n+\t\tdup2(devnull_fd, 1);\n \t} else if (cmd->stdout_to_stderr) {\n \t\ts1 = dup(1);\n \t\tdup2(2, 1);\n-- \n1.5.5\n"},{"id":"88451","messageId":"48B2AFC2.20901@viscovery.net","threadId":"15195","inReplyTo":"E1KXawS-0001gg-Ty@fencepost.gnu.org","subject":"Re: [PATCH v2] fix start_command() bug when stdin is closed","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-25T13:12:34Z","receivedAt":"2008-08-25T13:12:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Paolo Bonzini schrieb:\n> There is a problem in the use of dup2+close in start_command()\n> when one or more of file descriptors 0/1/2 are closed.\n\n\"Karl Chen pointed out a problem...\" (just to give due credit).\n\n> int start_command(struct child_process *cmd)\n> {\n> \tint need_in, need_out, need_err;\n> \tint fdin[2], fdout[2], fderr[2];\n> \n> \t/*\n> +\t * Make sure that all file descriptors <= 2 are open, otherwise we\n> +\t * mess them up when dup'ing pipes onto stdin/stdout/stderr. Since\n> +\t * we are at it, open a file descriptor on /dev/null to use it later.\n> +\t */\n> +\tif (devnull_fd == -1)\n> +\t {\n> +\t devnull_fd = open(\"/dev/null\", O_RDWR);\n> +\t while (devnull_fd >= 0 && devnull_fd <= 2)\n> +\t devnull_fd = dup(devnull_fd);\n> +\t if (devnull_fd == -1)\n> +\t die(\"opening /dev/null failed (%s)\", strerror(errno));\n> +\t }\n\nExcept for the insane GNU style indentation ;-) this makes a lot of sense.\n\nAcked-by: Johannes Sixt \n\nThe changes to the MINGW32 section are good (they pass the test suite).\nThanks for taking care of that.\n\n-- Hannes\n"},{"id":"88455","messageId":"E1KXcH3-0000zJ-0m@fencepost.gnu.org","threadId":"15195","inReplyTo":"48B2AFC2.20901@viscovery.net","subject":"[PATCH v2 properly indented] fix start_command() bug when stdin is closed","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-25T13:37:35Z","receivedAt":"2008-08-25T13:37:35Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"Karl Chen pointed out a problem in the use of dup2+close in\nstart_command() when one or more of file descriptors 0/1/2 are closed.\nIn order to rename a pipe file descriptor to 0, it does\n\n dup2(from, 0);\n close(from);\n\n... but if stdin was closed (for example) from == 0, so that\n\n dup2(0, 0);\n close(0);\n\njust ends up closing the pipe. This patch fixes it by opening all of\nthe \"low\" descriptors to /dev/null.\n\nIn most cases this patch will not cause any additional system calls;\nactually by reusing the /dev/null descriptor when possible (instead\nof opening a fresh one in dup_devnull) it may even save a handful in\nsome cases. :-)\n\nSigned-off-by: Paolo Bonzini \nAcknowledged-by: Johannes Sixt \n---\n run-command.c | 35 ++++++++++++++++++++++-------------\n 1 files changed, 22 insertions(+), 13 deletions(-)\n\n\t> \"Karl Chen pointed out a problem...\" (just to give due credit).\n\n\tOf course.\n\n\t> Except for the insane GNU style indentation ;-)\n\n\t*blush* -- both problems deriving from too hasty e-mail cut&paste.\n\n\t> this makes a lot of sense. [...] MINGW32 [...] pass the test suite.\n\n\tThanks, also for testing Windows.\n\n\tPaolo\n\ndiff --git a/run-command.c b/run-command.c\nindex caab374..4619494 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -2,25 +2,33 @@\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n \n+static int devnull_fd = -1;\n+\n static inline void close_pair(int fd[2])\n {\n \tclose(fd[0]);\n \tclose(fd[1]);\n }\n \n-static inline void dup_devnull(int to)\n-{\n-\tint fd = open(\"/dev/null\", O_RDWR);\n-\tdup2(fd, to);\n-\tclose(fd);\n-}\n-\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n \tint fdin[2], fdout[2], fderr[2];\n \n \t/*\n+\t * Make sure that all file descriptors <= 2 are open, otherwise we\n+\t * mess them up when dup'ing pipes onto stdin/stdout/stderr. Since\n+\t * we are at it, save a file descriptor on /dev/null to use it later.\n+\t */\n+\tif (devnull_fd == -1) {\n+\t\tdevnull_fd = open(\"/dev/null\", O_RDWR);\n+\t\twhile (devnull_fd >= 0 && devnull_fd <= 2)\n+\t\t\tdevnull_fd = dup(devnull_fd);\n+\t\tif (devnull_fd == -1)\n+\t\t\tdie(\"opening /dev/null failed (%s)\", strerror(errno));\n+\t}\n+\n+\t/*\n \t * In case of errors we must keep the promise to close FDs\n \t * that have been passed in via ->in and ->out.\n \t */\n@@ -72,7 +81,7 @@ int start_command(struct child_process *cmd)\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n \t\tif (cmd->no_stdin)\n-\t\t\tdup_devnull(0);\n+\t\t\tdup2(devnull_fd, 0);\n \t\telse if (need_in) {\n \t\t\tdup2(fdin[0], 0);\n \t\t\tclose_pair(fdin);\n@@ -82,14 +91,14 @@ int start_command(struct child_process *cmd)\n \t\t}\n \n \t\tif (cmd->no_stderr)\n-\t\t\tdup_devnull(2);\n+\t\t\tdup2(devnull_fd, 2);\n \t\telse if (need_err) {\n \t\t\tdup2(fderr[1], 2);\n \t\t\tclose_pair(fderr);\n \t\t}\n \n \t\tif (cmd->no_stdout)\n-\t\t\tdup_devnull(1);\n+\t\t\tdup2(devnull_fd, 1);\n \t\telse if (cmd->stdout_to_stderr)\n \t\t\tdup2(2, 1);\n \t\telse if (need_out) {\n@@ -127,7 +136,7 @@ int start_command(struct child_process *cmd)\n \n \tif (cmd->no_stdin) {\n \t\ts0 = dup(0);\n-\t\tdup_devnull(0);\n+\t\tdup2(devnull_fd, 0);\n \t} else if (need_in) {\n \t\ts0 = dup(0);\n \t\tdup2(fdin[0], 0);\n@@ -138,7 +147,7 @@ int start_command(struct child_process *cmd)\n \n \tif (cmd->no_stderr) {\n \t\ts2 = dup(2);\n-\t\tdup_devnull(2);\n+\t\tdup2(devnull_fd, 2);\n \t} else if (need_err) {\n \t\ts2 = dup(2);\n \t\tdup2(fderr[1], 2);\n@@ -146,7 +155,7 @@ int start_command(struct child_process *cmd)\n \n \tif (cmd->no_stdout) {\n \t\ts1 = dup(1);\n-\t\tdup_devnull(1);\n+\t\tdup2(devnull_fd, 1);\n \t} else if (cmd->stdout_to_stderr) {\n \t\ts1 = dup(1);\n \t\tdup2(2, 1);\n-- \n1.5.5\n"},{"id":"88469","messageId":"quack.20080825T0856.lth63ppulyu@roar.cs.berkeley.edu","threadId":"15195","inReplyTo":"48B28CF8.2060306@viscovery.net","subject":"Re: [PATCH] Fix start_command() pipe bug when stdin is closed.","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-25T15:56:57Z","receivedAt":"2008-08-25T15:56:57Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-08-25 03:44 PDT, Johannes Sixt writes:\n\n Johannes> When I try these instructions I don't get an error;\n Johannes> instead the command runs successfully.\n\nWeird. I see the symptom on two machines, both 1.5.6 and tracking\nmaster. The 1.5.6 system installation could be interfering even\nthough I used PATH=/path/to/git:$PATH.\n\n Johannes> While I do see that there is a problem, it is only\n Johannes> half of the story, and your patch addresses only\n Johannes> this half.\n\n Johannes> What if stdout is closed, too? Then the ends of the\n Johannes> first allocated pipe would go to fds 0 and 1, and\n Johannes> then the pipe end at 1 would be closed by a\n Johannes> subsequent dup2(xxx, 1).\n\nMy patch was intended to fix the problem for any renaming where\nfd_from==fd_to, including target stdout. I didn't say so in the\nchangelog though.\n"},{"id":"88470","messageId":"quack.20080825T0900.lth1w0dult2@roar.cs.berkeley.edu","threadId":"15195","inReplyTo":"E1KXcH3-0000zJ-0m@fencepost.gnu.org","subject":"Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-25T16:00:25Z","receivedAt":"2008-08-25T16:00:25Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":"\n>>>>> On 2008-08-25 06:37 PDT, Paolo Bonzini writes:\n\n Paolo> diff --git a/run-command.c b/run-command.c index\n Paolo> caab374..4619494 100644\n\nExcellent, this also fixes the problem for me.\n\nAcknowledged-by: Karl Chen \n\n(I guess that's the protocol...)\n\n\nWow, turnaround on this list sure is fast. Thanks, guys!\n"},{"id":"88528","messageId":"7v7ia4d4hq.fsf@gitster.siamese.dyndns.org","threadId":"15195","inReplyTo":"quack.20080825T0900.lth1w0dult2@roar.cs.berkeley.edu","subject":"Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-26T00:06:25Z","receivedAt":"2008-08-26T00:06:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen writes:\n\n>>>>>> On 2008-08-25 06:37 PDT, Paolo Bonzini writes:\n>\n> Paolo> diff --git a/run-command.c b/run-command.c index\n> Paolo> caab374..4619494 100644\n>\n> Excellent, this also fixes the problem for me.\n>\n> Acknowledged-by: Karl Chen \n>\n> (I guess that's the protocol...)\n>\n>\n> Wow, turnaround on this list sure is fast. Thanks, guys!\n\nThanks.\n"},{"id":"88545","messageId":"7vbpzgb94q.fsf@gitster.siamese.dyndns.org","threadId":"15195","inReplyTo":"E1KXcH3-0000zJ-0m@fencepost.gnu.org","subject":"Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-26T06:09:09Z","receivedAt":"2008-08-26T06:09:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paolo Bonzini writes:\n\n> int start_command(struct child_process *cmd)\n> {\n> \tint need_in, need_out, need_err;\n> \tint fdin[2], fdout[2], fderr[2];\n> \n> \t/*\n> +\t * Make sure that all file descriptors <= 2 are open, otherwise we\n> +\t * mess them up when dup'ing pipes onto stdin/stdout/stderr. Since\n> +\t * we are at it, save a file descriptor on /dev/null to use it later.\n> +\t */\n> +\tif (devnull_fd == -1) {\n> +\t\tdevnull_fd = open(\"/dev/null\", O_RDWR);\n> +\t\twhile (devnull_fd >= 0 && devnull_fd <= 2)\n> +\t\t\tdevnull_fd = dup(devnull_fd);\n> +\t\tif (devnull_fd == -1)\n> +\t\t\tdie(\"opening /dev/null failed (%s)\", strerror(errno));\n> +\t}\n> +\n\nI may be misreading the patch but, this logic always opens /dev/null, if\nnobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and\nworse, it keeps fd=3 open.\n\nMaking sure low fds 0, 1 and 2 are open is a good thing. I do not think\nclobbering fd=3 is good.\n\nAlso shouldn't this be done only on the side that dup()s fds around,\ni.e. in the child process after fork()? Why is this done for the parent?\n"},{"id":"88546","messageId":"48B3A3CC.3060906@viscovery.net","threadId":"15195","inReplyTo":"7vbpzgb94q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-26T06:33:48Z","receivedAt":"2008-08-26T06:33:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Paolo Bonzini writes:\n> \n>> int start_command(struct child_process *cmd)\n>> {\n>> \tint need_in, need_out, need_err;\n>> \tint fdin[2], fdout[2], fderr[2];\n>> \n>> \t/*\n>> +\t * Make sure that all file descriptors <= 2 are open, otherwise we\n>> +\t * mess them up when dup'ing pipes onto stdin/stdout/stderr. Since\n>> +\t * we are at it, save a file descriptor on /dev/null to use it later.\n>> +\t */\n>> +\tif (devnull_fd == -1) {\n>> +\t\tdevnull_fd = open(\"/dev/null\", O_RDWR);\n>> +\t\twhile (devnull_fd >= 0 && devnull_fd <= 2)\n>> +\t\t\tdevnull_fd = dup(devnull_fd);\n>> +\t\tif (devnull_fd == -1)\n>> +\t\t\tdie(\"opening /dev/null failed (%s)\", strerror(errno));\n>> +\t}\n>> +\n> \n> I may be misreading the patch but, this logic always opens /dev/null, if\n> nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and\n> worse, it keeps fd=3 open.\n> \n> Making sure low fds 0, 1 and 2 are open is a good thing. I do not think\n> clobbering fd=3 is good.\n\nIt is sometimes _unnecessary_, but I don't see why it should hurt. The\neffect on performance will be in the noise.\n\n> Also shouldn't this be done only on the side that dup()s fds around,\n> i.e. in the child process after fork()? Why is this done for the parent?\n\nBecause it must be done *before* the pipe()s are created so that they\ndon't occupy fds 0-2.\n\n-- Hannes\n"},{"id":"88548","messageId":"48B3A679.2050103@gnu.org","threadId":"15195","inReplyTo":"7vbpzgb94q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-26T06:45:13Z","receivedAt":"2008-08-26T06:45:13Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"Hannes already answered everything, but now that I think more about it I\nwould actually consider putting it in main, just in case an important\nfile ends up in file descriptor = 2 and is corrupted by a call to die().\n Just in case, this program shows that stderr always point to fd 2 even\nif it is closed upon launch:\n\n #include \n #include \n\n int main()\n {\n int fd = open (\"/dev/tty\", O_WRONLY);\n FILE *fp;\n fp = fdopen (fd, \"w\");\n fprintf (fp, \"file descriptor %d\\n\", fd);\n fflush (fp);\n fprintf (stderr, \"writing on stderr now\\n\");\n }\n\n bonzinip$ ./a.out 2<&-\n file descriptor 2\n writing on stderr now\n bonzinip$\n\nPatch coming in a moment.\n\nPaolo\n"},{"id":"88549","messageId":"E1KXsL9-0004ef-Co@fencepost.gnu.org","threadId":"15195","inReplyTo":"7vbpzgb94q.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-26T06:48:35Z","receivedAt":"2008-08-26T06:48:35Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"It is in general unsafe to start git with one or more of file descriptors\n0/1/2 closed. Karl Chen for example noticed that stat_command does this\nin order to rename a pipe file descriptor to 0:\n\n dup2(from, 0);\n close(from);\n\n... but if stdin was closed (for example) from == 0, so that\n\n dup2(0, 0);\n close(0);\n\njust ends up closing the pipe. Another extremely rare but nasty problem\nwould occur if an \"important\" file ends up in file descriptor 2, and is\ncorrupted by a call to die().\n\nThis patch fixes these problems by opening all of the \"low\" descriptors\nto /dev/null in main.\n\nSigned-off-by: Paolo Bonzini \n---\n git.c | 13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 89e4645..be227b2 100644\n--- a/git.c\n+++ b/git.c\n@@ -420,6 +420,19 @@ int main(int argc, const char **argv)\n \tconst char *cmd = argv[0] && *argv[0] ? argv[0] : \"git-help\";\n \tchar *slash = (char *)cmd + strlen(cmd);\n \tint done_alias = 0;\n+\tint devnull_fd;\n+\n+\t/*\n+\t * Always open file descriptors 0/1/2 to avoid clobbering files\n+\t * in die(). It also avoids not messing up when the pipes are\n+\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n+\t */\n+\tdevnull_fd = open(\"/dev/null\", O_RDWR);\n+\twhile (devnull_fd >= 0 && devnull_fd <= 2)\n+\t\tdevnull_fd = dup(devnull_fd);\n+\tif (devnull_fd == -1)\n+\t\tdie(\"opening /dev/null failed (%s)\", strerror(errno));\n+\tclose (devnull_fd);\n \n \t/*\n \t * Take the basename of argv[0] as the command\n-- \n1.5.5\n"},{"id":"88551","messageId":"48B3A948.3080800@viscovery.net","threadId":"15195","inReplyTo":"E1KXsL9-0004ef-Co@fencepost.gnu.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-26T06:57:12Z","receivedAt":"2008-08-26T06:57:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Paolo Bonzini schrieb:\n> +\t/*\n> +\t * Always open file descriptors 0/1/2 to avoid clobbering files\n> +\t * in die(). It also avoids not messing up when the pipes are\n> +\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n> +\t */\n\nI see your point, but I don't have an opinion whether this stretch is\nnecessary.\n\nHowever, *if* we do this, we must do it for all non-builtins as well!\n\n-- Hannes\n"},{"id":"88554","messageId":"20080826074044.GA22694@cuci.nl","threadId":"15195","inReplyTo":"48B3A948.3080800@viscovery.net","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-26T07:40:44Z","receivedAt":"2008-08-26T07:40:44Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Johannes Sixt wrote:\n>Paolo Bonzini schrieb:\n>> +\t/*\n>> +\t * Always open file descriptors 0/1/2 to avoid clobbering files\n>> +\t * in die(). It also avoids not messing up when the pipes are\n>> +\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n>> +\t */\n\n>I see your point, but I don't have an opinion whether this stretch is\n>necessary.\n>However, *if* we do this, we must do it for all non-builtins as well!\n\nWell, in general the policy I've used in all the tools I created is that:\n\na. If it's a setuid tool, then you need to make sure that you don't step\n on anything unintendedly. I.e. for setuid-something programs this is\n desirable and necessary in order to prevent securityleaks.\n\nb. Anything else is started in an environment controlled by the user,\n and if this environment is broken, then that is the user's fault.\n You get what you wish for. It's a similar problem you get when you\n set PATH to wrong values and then start \"make\" for example; it has\n the potential to break a lot; but then again there are infinitely\n more ways to shoot yourself in the foot, than there are ways to\n prevent people from shooting in some particular way.\n\nSo I'd say, if the tools are setuid (which none of git's tools are) and\nare therefore potentially started from a hostile and uncontrolled\nenvironment, please make sure filedescriptors 0, 1 and 2 are sane.\nBut for the git utilities, it would be a non-watertight extra safeguard\nwhich tries to prevent a situation which rarely occurs and if it does\noccur, you probably are doing some other things wrong as well; so\nactually exposing those problems to you by letting you feel the pain can\nbe considered a favour.\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"Good moaning!\"\n"},{"id":"88614","messageId":"7vsksrad7o.fsf@gitster.siamese.dyndns.org","threadId":"15195","inReplyTo":"48B3A948.3080800@viscovery.net","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-26T17:38:35Z","receivedAt":"2008-08-26T17:38:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Paolo Bonzini schrieb:\n>> +\t/*\n>> +\t * Always open file descriptors 0/1/2 to avoid clobbering files\n>> +\t * in die(). It also avoids not messing up when the pipes are\n>> +\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n>> +\t */\n>\n> I see your point, but I don't have an opinion whether this stretch is\n> necessary.\n\nThis is going too far. Have you seen any other sane program that do this?\n"},{"id":"88628","messageId":"48B44C61.2020206@gnu.org","threadId":"15195","inReplyTo":"7vsksrad7o.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-26T18:33:05Z","receivedAt":"2008-08-26T18:33:05Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"Junio C Hamano wrote:\n> Johannes Sixt writes:\n> \n>> Paolo Bonzini schrieb:\n>>> +\t/*\n>>> +\t * Always open file descriptors 0/1/2 to avoid clobbering files\n>>> +\t * in die(). It also avoids not messing up when the pipes are\n>>> +\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n>>> +\t */\n>> I see your point, but I don't have an opinion whether this stretch is\n>> necessary.\n> \n> This is going too far. Have you seen any other sane program that do this?\n\nBusybox. But it runs setuid, as Steven pointed out.\n\nI say it's all (i.e. be this paranoid), or nothing.\n\nPaolo\n"},{"id":"88661","messageId":"7vabez2yac.fsf@gitster.siamese.dyndns.org","threadId":"15195","inReplyTo":"48B44C61.2020206@gnu.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-26T22:42:51Z","receivedAt":"2008-08-26T22:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paolo Bonzini writes:\n\n> Junio C Hamano wrote:\n>> Johannes Sixt writes:\n>> \n>>> Paolo Bonzini schrieb:\n>>>> +\t/*\n>>>> +\t * Always open file descriptors 0/1/2 to avoid clobbering files\n>>>> +\t * in die(). It also avoids not messing up when the pipes are\n>>>> +\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n>>>> +\t */\n>>> I see your point, but I don't have an opinion whether this stretch is\n>>> necessary.\n>> \n>> This is going too far. Have you seen any other sane program that do this?\n>\n> Busybox. But it runs setuid, as Steven pointed out.\n>\n> I say it's all (i.e. be this paranoid), or nothing.\n\nI tend to agree, and I think what Stephen R. van den Berg said earlier in\nthe thread makes perfect sense.\n"},{"id":"88663","messageId":"7v3akr2xa3.fsf@gitster.siamese.dyndns.org","threadId":"15195","inReplyTo":"7vabez2yac.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-26T23:04:36Z","receivedAt":"2008-08-26T23:04:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Paolo Bonzini writes:\n>\n>> Junio C Hamano wrote:\n>>> Johannes Sixt writes:\n>>> \n>>>> Paolo Bonzini schrieb:\n>>>>> +\t/*\n>>>>> +\t * Always open file descriptors 0/1/2 to avoid clobbering files\n>>>>> +\t * in die(). It also avoids not messing up when the pipes are\n>>>>> +\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n>>>>> +\t */\n>>>> I see your point, but I don't have an opinion whether this stretch is\n>>>> necessary.\n>>> \n>>> This is going too far. Have you seen any other sane program that do this?\n>>\n>> Busybox. But it runs setuid, as Steven pointed out.\n>>\n>> I say it's all (i.e. be this paranoid), or nothing.\n>\n> I tend to agree, and I think what Stephen R. van den Berg said earlier in\n> the thread makes perfect sense.\n\nSo going back to the very original in the thread.\n\nI think\n\n\t$ git fetch 0<&-\n\nfrom the command line is a mere user stupidity.\n\nOn the other hand, if a cron/at job that contains \"git fetch\" is launched\nin an environment with fd#0 (or #1 or #2 for that matter) closed, it would\ncertainly be problematic. It can easily be worked around by redirecting\nfile descriptors appropriately in the script that is launched, though.\n\nOn a related note, we should make sure that we run our hooks with the set\nof low file descriptors opened sensibly. It would be a bug if we are\nrunning them in a weird environment and forcing them to do funky\nredirection themselves. I think we are already Ok in this regard, but I\ndidn't check.\n"},{"id":"88664","messageId":"20080826231038.GA24323@cuci.nl","threadId":"15195","inReplyTo":"7v3akr2xa3.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-26T23:10:38Z","receivedAt":"2008-08-26T23:10:38Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>Junio C Hamano writes:\n>I think\n>\t$ git fetch 0<&-\n>from the command line is a mere user stupidity.\n\n>On the other hand, if a cron/at job that contains \"git fetch\" is launched\n>in an environment with fd#0 (or #1 or #2 for that matter) closed, it would\n>certainly be problematic. It can easily be worked around by redirecting\n>file descriptors appropriately in the script that is launched, though.\n\nA sane cron environment always has proper 0, 1 and 2 descriptors.\nThis basically goes with rule #2: if your cron doesn't have 0, 1 and 2\nopen, you have big problems already, so camouflaging those problems\nis not going to help the user.\n\n>On a related note, we should make sure that we run our hooks with the set\n>of low file descriptors opened sensibly. It would be a bug if we are\n>running them in a weird environment and forcing them to do funky\n>redirection themselves. I think we are already Ok in this regard, but I\n>didn't check.\n\nAgreed, but this is the responsibility of anyone launching other\nprocesses (cleanup, then launch).\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"Good moaning!\"\n"},{"id":"88745","messageId":"20080827020400.GA12189@mail.local.tull.net","threadId":"15195","inReplyTo":"7vsksrad7o.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Nick Andrew","fromEmail":"nick@nick-andrew.net","sentAt":"2008-08-27T02:04:00Z","receivedAt":"2008-08-27T02:04:00Z","isPatch":true,"sender":{"key":"nick@nick-andrew.net","avatar":"https://gravatar.com/avatar/85f25a67ca6eaa4016ed374f6d07f3cd853c886aeb7e1507eb7dbc47b00082fe?d=mp&s=160"},"body":"On Tue, Aug 26, 2008 at 10:38:35AM -0700, Junio C Hamano wrote:\n> This is going too far. Have you seen any other sane program that do this?\n\nHmm. I posted a patch to the \"sane\" project (scanner daemon) to avoid a\nsimilar problem. The patch was rejected.\n\nsaned tries to sanitise its environment, specifically low order fds:\n\nfd = open(\"/dev/null\", O_RDWR);\ndup2(fd, 0);\ndup2(fd, 1);\nfup2(fd, 2);\nclose(fd);\n\nAnd I pointed out that if the fds aren't sanitary (all fds open) before\nthe code snippet, they won't be sanitary after it. My patch was only:\n\nif (fd > 2)\n close(fd);\n\nIf closing fd 0/1/2 and then forking a subprocess is the unix equivalent\nof delayed shooting yourself in the foot then I can agree; git doesn't\nneed it.\n\nNick.\n"},{"id":"88679","messageId":"quack.20080826T2005.lthzlmz2m4g@roar.cs.berkeley.edu","threadId":"15195","inReplyTo":"7v3akr2xa3.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-08-27T03:05:35Z","receivedAt":"2008-08-27T03:05:35Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-08-26 16:04 PDT, Junio C Hamano writes:\n\n Junio> I think\n\n Junio> \t$ git fetch 0<&-\n\n Junio> from the command line is a mere user stupidity.\n\n Junio> On the other hand, if a cron/at job that contains \"git\n Junio> fetch\" is launched in an environment with fd#0 (or #1\n Junio> or #2 for that matter) closed, it would certainly be\n Junio> problematic. It can easily be worked around by\n Junio> redirecting file descriptors appropriately in the\n Junio> script that is launched, though.\n\nI agree command-line 'git fetch 0<&-' is silly. The example I\ngave was minimized to show the symptom. I ran across this with a\nmore complicated cron-ish setup that closes stdin. I actually had\nto look up the shell syntax for closing file descriptors.\n\nYes, I can work around this issue with sh -c 'git fetch\n0 Yes, I can work around this issue with sh -c 'git fetch\n> 0 But I don't see the harm in being safe. It's one less potential\n> surprise for users. This is the first program I've encountered\n> that broke due to stdin being closed\n\nNot really. I suspect every program that uses pipe/dup to fork a child\ncould be wrong (the only one I ever wrote breaks), and I wonder if the\nhigher-level popen(3) interface works properly.\n\nPaolo\n"},{"id":"88687","messageId":"32541b130808262201v4d7c1aa5r781720a80b79fcd0@mail.gmail.com","threadId":"15195","inReplyTo":"20080826074044.GA22694@cuci.nl","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-08-27T05:01:00Z","receivedAt":"2008-08-27T05:01:00Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On 8/26/08, Stephen R. van den Berg wrote:\n> Well, in general the policy I've used in all the tools I created is that:\n>\n> a. If it's a setuid tool, then you need to make sure that you don't step\n> on anything unintendedly. I.e. for setuid-something programs this is\n> desirable and necessary in order to prevent securityleaks.\n>\n> b. Anything else is started in an environment controlled by the user,\n> and if this environment is broken, then that is the user's fault.\n\nIn general I'd mostly agree with you, but fd 0/1/2 are super-special\nand I've personally been bitten by insane, rare problems that occur\nwhen programs are started with one or more of those fds closed.\n\nThe usual case is that you're writing a new daemon. The generally\naccepted behaviour for a daemon is to chdir(\"/') and then close all\nunnecessary open fds, in order to minimize the chance that it will be\nholding open any directories or files that would prevent unmounting a\nfilesystem. On the other hand, if the daemon then needs to run git\nfor some reason (who knows! maybe it's a git auto-commit daemon as was\ndiscussed earlier on the list), it needs to open file descriptors\ninstead. Such a program might work 99% of the time when git doesn't\nhappen to print any output. But if there's ever an error, git would\nprint to fd#2 on die(), and that could corrupt some random file that\nthe daemon *or* git was using. Remember, the situations where the\ndaemon leaves fd#2 open pointing at *the wrong thing* aren't the real\nproblem - you could easily say that's the daemon leaving the\nenvironment in an insane state. The problem situation is when git\nopened some random file, and it *happened* to get assigned fd#2, and\nthen git incorrectly assumed that writing to fd#2 would not corrupt a\nfile that it opened.\n\nDoes this sound rare? It is! But it's also hellish to debug when it\nhappens, precisely because of its rarity. For example, in one case, I\nhad this problem because an sfdisk process started by my custom\n/sbin/init ran into a minor warning, and printed it to fd#2.\nUnfortunately, because /sbin/init had opened sfdisk with fds 0/1/2\nclosed, fd#2 ended up being the very disk it was partitioning. The\nboot sector ended up getting overwritten with a warning message in\nsomething like 1 out of 100 cases, and the computer wouldn't boot.\nARGH. Easy to debug, once you think to read the boot sector as\nplaintext. But that's not the first thing you think to do.\n\nAnyway, I personally think that given how incredibly cheap this\noperation is to do, and how startlingly painful it is to debug when it\n*is* a problem, that it would be nice if every program just did this\nby default. I would personally feel fine if such a thing ended up in\nlibc or the kernel, although presumably that would violate POSIX.\n\nYes, it would also be fine to have every *daemon* make sure it opens\n/dev/null instead of just closing fd 0/1/2. But it's harmless to have\nboth.\n\n(As for the non-builtin git commands, isn't this an advantage of\nhaving everything get run through the main /usr/bin/git wrapper?)\n\nHave fun,\n\nAvery\n"},{"id":"88692","messageId":"48B4F5C4.9020404@viscovery.net","threadId":"15195","inReplyTo":"48B44C61.2020206@gnu.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-27T06:35:48Z","receivedAt":"2008-08-27T06:35:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Paolo Bonzini schrieb:\n> Junio C Hamano wrote:\n>> Johannes Sixt writes:\n>>\n>>> Paolo Bonzini schrieb:\n>>>> +\t/*\n>>>> +\t * Always open file descriptors 0/1/2 to avoid clobbering files\n>>>> +\t * in die(). It also avoids not messing up when the pipes are\n>>>> +\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n>>>> +\t */\n>>> I see your point, but I don't have an opinion whether this stretch is\n>>> necessary.\n>> This is going too far. Have you seen any other sane program that do this?\n> \n> Busybox. But it runs setuid, as Steven pointed out.\n\nI straced tee (it was the only tool I found that opens files for writing\nwithout also opening some for reading). If one of 0,1,2 is closed, it\n*does* dup() the fd that it is going to write.\n\nDon't you now feel like Reg in \"Life of Brian\":\n\n\"All right, but apart from the sanitation, the medicine, education, wine,\npublic order, irrigation, roads, a fresh water system, and public health,\nwhat have the Romans ever done for us?\"\n\n;)\n\n-- Hannes\n"},{"id":"88703","messageId":"48B50E47.3010402@gnu.org","threadId":"15195","inReplyTo":"48B4F5C4.9020404@viscovery.net","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-27T08:20:23Z","receivedAt":"2008-08-27T08:20:23Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"\n>> Busybox. But it runs setuid, as Steven pointed out.\n> \n> I straced tee (it was the only tool I found that opens files for writing\n> without also opening some for reading). If one of 0,1,2 is closed, it\n> *does* dup() the fd that it is going to write.\n\nTo be precise, it does a blind \"dup2 (fd, 3)\" and goes on with file\ndescriptor 3.\n\nopen(\"foo\", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0666) = 0\nfcntl64(0, F_DUPFD, 3) = 3\nclose(0) = 0\n\nPaolo\n"},{"id":"88711","messageId":"20080827090423.GA484@cuci.nl","threadId":"15195","inReplyTo":"quack.20080826T2005.lthzlmz2m4g@roar.cs.berkeley.edu","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-27T09:04:23Z","receivedAt":"2008-08-27T09:04:23Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Karl Chen wrote:\n>Yes, I can work around this issue with sh -c 'git fetch\n>0But I don't see the harm in being safe. It's one less potential\n>surprise for users. This is the first program I've encountered\n>that broke due to stdin being closed, and it took debugging to\n>figure out that was the reason.\n\nI understand the reasoning, and there sure is a valid point in here\n(principle of least surprise), but there is also the case of hiding\nproblems.\nIt's a bit unclear which should prevail here.\nThe point is that if you actually make git detect and correct\nclosed descriptors which should have been open, then you are merely\npassing the buck to all other programs the user is starting which might\nor might not break.\n\nMaybe the breakage of other programs is only in conjunction with\nfull-moon and FD 0 closed, in that case you make the problems/bugs even\n*harder* to find for the user by making git \"fix it for you\".\n-- \nSincerely,\n Stephen R. van den Berg.\n\"First, God created idiots. That was just for practice.\n Then he created school boards.\" -- Mark Twain\n"},{"id":"88712","messageId":"20080827091800.GB484@cuci.nl","threadId":"15195","inReplyTo":"32541b130808262201v4d7c1aa5r781720a80b79fcd0@mail.gmail.com","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-27T09:18:00Z","receivedAt":"2008-08-27T09:18:00Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Avery Pennarun wrote:\n>On 8/26/08, Stephen R. van den Berg wrote:\n>> Well, in general the policy I've used in all the tools I created is that:\n\n>> a. If it's a setuid tool, then you need to make sure that you don't step\n>> on anything unintendedly. I.e. for setuid-something programs this is\n>> desirable and necessary in order to prevent securityleaks.\n\n>> b. Anything else is started in an environment controlled by the user,\n>> and if this environment is broken, then that is the user's fault.\n\n>In general I'd mostly agree with you, but fd 0/1/2 are super-special\n>and I've personally been bitten by insane, rare problems that occur\n>when programs are started with one or more of those fds closed.\n\nKey words: \"insane, rare problems\"\n\n>The usual case is that you're writing a new daemon. The generally\n>accepted behaviour for a daemon is to chdir(\"/') and then close all\n\n>the daemon *or* git was using. Remember, the situations where the\n>daemon leaves fd#2 open pointing at *the wrong thing* aren't the real\n>problem - you could easily say that's the daemon leaving the\n>environment in an insane state. The problem situation is when git\n\nWell, as you say, \"you're writing a new daemon\". This means that you\nneed to make sure that *if* this daemon ever forks/execs it leaves the\nenvironment in a sane state which does not open up security holes.\nThat means that you need to sanitise the environment, that you need to\nkeep tabs on all the descriptors that need to be closed on exec, and\nthat it needs to make sure that fd 0, 1 and 2 are pointing to somewhere\nappriopriate (/dev/null, if nothing else).\nThe fact that you forgot to do some of those things means that it is\nvery helpful if you discover this fact as soon as possible. By making\ngit \"fix it for you\" you will not notice any problems when running git\nfrom your daemon. Nonetheless this will not cause you to fix your\ncode.\n\n>Does this sound rare? It is! But it's also hellish to debug when it\n>happens, precisely because of its rarity. For example, in one case, I\n>had this problem because an sfdisk process started by my custom\n\nThing is, by making git (and some other programs) hide this problem\nfrom you, this problem will get even *harder* to debug. Whereas as a\ndaemon author you should be thankful that something breaks and shows you\nyour daemon needs fixing.\n\n>Anyway, I personally think that given how incredibly cheap this\n>operation is to do, and how startlingly painful it is to debug when it\n>*is* a problem, that it would be nice if every program just did this\n>by default. I would personally feel fine if such a thing ended up in\n>libc or the kernel, although presumably that would violate POSIX.\n\nAnd then you'd have the situation that in some cases, where this\nmechanism is bypassed or not present, various daemons might show security\nholes in their filedescriptor management which nobody noticed before.\n\n>Yes, it would also be fine to have every *daemon* make sure it opens\n>/dev/null instead of just closing fd 0/1/2. \n\nIt would not only be fine, it *is* required, since 1972.\n\n> But it's harmless to have\n>both.\n\nConsidering the fact that daemon authors might not get pointed at their\nmistakes as soon as possible, it is harmful to try and hide those facts.\n\n>(As for the non-builtin git commands, isn't this an advantage of\n>having everything get run through the main /usr/bin/git wrapper?)\n\nAt best a program (e.g. git) could give off a warning when it finds\nthe filedescriptors in a bad state (and then fix it), but that would\nmean that from within git we'd be trying to save the world, since anyone\nnot running git would not get those warnings. You have to draw the line\nsomewhere, and for user-tools it ends here; for setuid-tools it's\ndifferent: they need to detect and fix it anyway, and therefore could easily\nwarn as well.\n-- \nSincerely,\n Stephen R. van den Berg.\n\"First, God created idiots. That was just for practice.\n Then he created school boards.\" -- Mark Twain\n"},{"id":"88722","messageId":"48B54A3D.3080708@gnu.org","threadId":"15195","inReplyTo":"20080827091800.GB484@cuci.nl","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-27T12:36:13Z","receivedAt":"2008-08-27T12:36:13Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"\n>> But it's harmless to have both.\n> \n> Considering the fact that daemon authors might not get pointed at their\n> mistakes as soon as possible, it is harmful to try and hide those facts.\n\nAgree. OTOH what about opening fd's 0/1/2 to /dev/null only in\ngit-shell.c, now that it's not a builtin anymore?\n\nMaybe it does not fix Karl's use case, but it seems sensible to me.\n\nPaolo\n"},{"id":"88728","messageId":"E1KYMtm-0007Cd-Gt@fencepost.gnu.org","threadId":"15195","inReplyTo":"48B54A3D.3080708@gnu.org","subject":"[PATCH v4] make git-shell paranoid about closed stdin/stdout/stderr","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-27T15:20:35Z","receivedAt":"2008-08-27T15:20:35Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"It is in general unsafe to start a program with one or more of file\ndescriptors 0/1/2 closed. Karl Chen for example noticed that stat_command\ndoes this in order to rename a pipe file descriptor to 0:\n\n dup2(from, 0);\n close(from);\n\n... but if stdin was closed (for example) from == 0, so that\n\n dup2(0, 0);\n close(0);\n\njust ends up closing the pipe. Another extremely rare but nasty problem\nwould occur if an \"important\" file ends up in file descriptor 2, and is\ncorrupted by a call to die().\n\nFixing this in git was considered to be overkill, so this patch works\naround it only for git-shell. The fix is simply to open all the \"low\"\ndescriptors to /dev/null in main.\n\nSigned-off-by: Paolo Bonzini \n---\n shell.c | 13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/shell.c b/shell.c\nindex 0f6a727..e339369 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -48,6 +48,19 @@ int main(int argc, char **argv)\n {\n \tchar *prog;\n \tstruct commands *cmd;\n+\tint devnull_fd;\n+\n+\t/*\n+\t * Always open file descriptors 0/1/2 to avoid clobbering files\n+\t * in die(). It also avoids not messing up when the pipes are\n+\t * dup'ed onto stdin/stdout/stderr in the child processes we spawn.\n+\t */\n+\tdevnull_fd = open(\"/dev/null\", O_RDWR);\n+\twhile (devnull_fd >= 0 && devnull_fd <= 2)\n+\t\tdevnull_fd = dup(devnull_fd);\n+\tif (devnull_fd == -1)\n+\t\tdie(\"opening /dev/null failed (%s)\", strerror(errno));\n+\tclose (devnull_fd);\n \n \t/*\n \t * Special hack to pretend to be a CVS server\n-- \n1.5.5\n"},{"id":"88735","messageId":"20080827172206.GA27450@cuci.nl","threadId":"15195","inReplyTo":"E1KYMtm-0007Cd-Gt@fencepost.gnu.org","subject":"Re: [PATCH v4] make git-shell paranoid about closed stdin/stdout/stderr","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-27T17:22:06Z","receivedAt":"2008-08-27T17:22:06Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Paolo Bonzini wrote:\n>Fixing this in git was considered to be overkill, so this patch works\n>around it only for git-shell. The fix is simply to open all the \"low\"\n>descriptors to /dev/null in main.\n\nSince git-shell is not setuid, this strictly is not necessary, however,\nI concur that git-shell is potentially started in a partially broken\nenvironment which is not always easily fixable by the user.\nAnd since git-shell needs to sanitise the filedescriptors anyway before\nlaunching other programs, it might as well cleanup at startup if needed.\n\nAcked-by: Stephen R. van den Berg \n-- \nSincerely,\n Stephen R. van den Berg.\n\"First, God created idiots. That was just for practice.\n Then he created school boards.\" -- Mark Twain\n"},{"id":"88736","messageId":"7vej4aqsge.fsf@gitster.siamese.dyndns.org","threadId":"15195","inReplyTo":"48B54A3D.3080708@gnu.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-27T17:27:13Z","receivedAt":"2008-08-27T17:27:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paolo Bonzini writes:\n\n>>> But it's harmless to have both.\n>> \n>> Considering the fact that daemon authors might not get pointed at their\n>> mistakes as soon as possible, it is harmful to try and hide those facts.\n>\n> Agree. OTOH what about opening fd's 0/1/2 to /dev/null only in\n> git-shell.c, now that it's not a builtin anymore?\n\nHmm, why git-shell?\n\nIt is either run by ssh (via command=\"\" option in authorized_keys file),\nby init/login (if in /etc/passwd), or by gitosis (and its equivalent).\n\nWouldn't these callers already give it a sane environment (and if a\nlookalike to gitosis forgets to do so, wouldn't Stephen's argument not to\nhide the issue from the daemon writers apply)?\n"},{"id":"88746","messageId":"32541b130808271122t45031cc7n497da8da6ca52bd3@mail.gmail.com","threadId":"15195","inReplyTo":"20080827091800.GB484@cuci.nl","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-08-27T18:22:39Z","receivedAt":"2008-08-27T18:22:39Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Wed, Aug 27, 2008 at 5:18 AM, Stephen R. van den Berg wrote:\n> Avery Pennarun wrote:\n>>In general I'd mostly agree with you, but fd 0/1/2 are super-special\n>>and I've personally been bitten by insane, rare problems that occur\n>>when programs are started with one or more of those fds closed.\n>\n> Key words: \"insane, rare problems\"\n\nYes, I used those words on purpose.\n\n> Well, as you say, \"you're writing a new daemon\". This means that you\n> need to make sure that *if* this daemon ever forks/execs it leaves the\n> environment in a sane state which does not open up security holes.\n\nWell, *I* know that. But this is far from well-documented.\n\n>>Does this sound rare? It is! But it's also hellish to debug when it\n>>happens, precisely because of its rarity. For example, in one case, I\n>>had this problem because an sfdisk process started by my custom\n>\n> Thing is, by making git (and some other programs) hide this problem\n> from you, this problem will get even *harder* to debug. Whereas as a\n> daemon author you should be thankful that something breaks and shows you\n> your daemon needs fixing.\n\nTrue enough, unless it was worked around in libc or the kernel as I\nsuggested. That said, if git opens a file and writes random log\nmessages to it, I'd still consider that to be git's fault for doing\nso.\n\nI'm just feeling protective of the future sanity of other developers\nhere, hoping they don't have to go through what I did on a multi-week\nbug hunt. (We were even blaming reiserfs for a while for our boot\nsector getting zapped...) The fact that someone *other* than me has\nsuggested this change implies that I'm not the only one who has seen\nsuch insanity in the wild.\n\nIt'd be fine if git simply died if fd 0, 1, or 2 isn't open when it\nstarts. Printing a warning message wouldn't work, for hopefully\nobvious reasons. But it would be a shame to simply ignore this sort\nof problem now that it's been brought up.\n\nHave fun,\n\nAvery\n"},{"id":"88873","messageId":"20080828122142.GA6518@mail.local.tull.net","threadId":"15195","inReplyTo":"32541b130808271122t45031cc7n497da8da6ca52bd3@mail.gmail.com","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Nick Andrew","fromEmail":"nick@nick-andrew.net","sentAt":"2008-08-28T12:21:42Z","receivedAt":"2008-08-28T12:21:42Z","isPatch":true,"sender":{"key":"nick@nick-andrew.net","avatar":"https://gravatar.com/avatar/85f25a67ca6eaa4016ed374f6d07f3cd853c886aeb7e1507eb7dbc47b00082fe?d=mp&s=160"},"body":"On Wed, Aug 27, 2008 at 02:22:39PM -0400, Avery Pennarun wrote:\n> I'm just feeling protective of the future sanity of other developers\n> here, hoping they don't have to go through what I did on a multi-week\n> bug hunt. (We were even blaming reiserfs for a while for our boot\n> sector getting zapped...) The fact that someone *other* than me has\n> suggested this change implies that I'm not the only one who has seen\n> such insanity in the wild.\n\nYou're not alone. I've been having trouble with a combination of\nfetchmail, procmail and ssmtp, in which situation the ssmtp program\n_somehow_ sometimes opens /dev/urandom as file descriptor 0 (while\ncalculating an SSL key?) and leaves it open, then reads the message\nbody from that file descriptor, resulting in an endless garbage message\nbeing sent to the SMTP server.\n\nI suspect the error originates in Debian's patch to ssmtp (which\nadded the SSL support) but I haven't been able to reproduce the bug\nin controlled circumstances. It's possible that fetchmail or procmail\nis doing something stupid - but a little more defensive programming\nin ssmtp could avoid the total disaster area of sending an endless\nbinary stream to an SMTP server.\n\nSo although I'm not experiencing any problems with git due to incorrect\nfile descriptor usage, I'm sensitive to the general issue.\n\nNick.\n"},{"id":"88876","messageId":"20080828125258.GA16940@cuci.nl","threadId":"15195","inReplyTo":"20080828122142.GA6518@mail.local.tull.net","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-28T12:52:58Z","receivedAt":"2008-08-28T12:52:58Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Nick Andrew wrote:\n>On Wed, Aug 27, 2008 at 02:22:39PM -0400, Avery Pennarun wrote:\n>> I'm just feeling protective of the future sanity of other developers\n>> here, hoping they don't have to go through what I did on a multi-week\n\n>You're not alone. I've been having trouble with a combination of\n>fetchmail, procmail and ssmtp, in which situation the ssmtp program\n>_somehow_ sometimes opens /dev/urandom as file descriptor 0 (while\n\n>in controlled circumstances. It's possible that fetchmail or procmail\n>is doing something stupid - but a little more defensive programming\n>in ssmtp could avoid the total disaster area of sending an endless\n>binary stream to an SMTP server.\n\nProcmail I can vouch for, it basically assumes your OS is broken and\nfights it's way back to sanity (it can be setuid root, so it has to\nbe rather careful).\nNonetheless, I still maintain that hiding problems doesn't help, it\nonly makes the bugs even rarer and more difficult to find.\n\nThe filedescriptor problem is a programmer-error, not a user-error,\nwhich is why not hiding it should be preferred. If it were a\nuser-error, thing would be different, assisting the user is a Good\nThing.\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"Listen carefully, I shall say this only wence.\"\n"},{"id":"88883","messageId":"48B6A57A.6050109@gnu.org","threadId":"15195","inReplyTo":"7vej4aqsge.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-28T13:17:46Z","receivedAt":"2008-08-28T13:17:46Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"\n> It is either run by ssh (via command=\"\" option in authorized_keys file),\n> by init/login (if in /etc/passwd), or by gitosis (and its equivalent).\n\nIt is possible to run it with file descriptors closed via ssh, using\ncommand=\"git-shell 0<&- 1<&- 2<&-\" in the authorized_keys file.\n\nIt's true that in this case the user is also shooting himself, but given\nthat git-shell is used to restrict operation to \"safe\" commands, this\nspecial case might be worth being worked around.\n\nPaolo\n"},{"id":"88887","messageId":"20080828135846.GA6874@cuci.nl","threadId":"15195","inReplyTo":"48B6A57A.6050109@gnu.org","subject":"Re: [PATCH] be paranoid about closed stdin/stdout/stderr","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-28T13:58:46Z","receivedAt":"2008-08-28T13:58:46Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Paolo Bonzini wrote:\n>> It is either run by ssh (via command=\"\" option in authorized_keys file),\n>> by init/login (if in /etc/passwd), or by gitosis (and its equivalent).\n\n>It is possible to run it with file descriptors closed via ssh, using\n>command=\"git-shell 0<&- 1<&- 2<&-\" in the authorized_keys file.\n\nI don't consider this that relevant, however...\n\n>It's true that in this case the user is also shooting himself, but given\n>that git-shell is used to restrict operation to \"safe\" commands, this\n>special case might be worth being worked around.\n\nSince a programmer error in this case doesn't inflict just pain on the\nuser, but also is a potential security leak that can potentially be \nexploited by third party users, things are different, and it is worth\ncatering for.\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"Listen carefully, I shall say this only wence.\"\n"}]}