{"thread":{"id":"4854","subject":"git-daemon problem","startedAt":"2006-07-11T22:24:24Z","lastAt":"2006-07-14T15:53:29Z","messageCount":23,"participants":["Matthias Lederhofer","Junio C Hamano","Andre Noll","Edgar Toernig","Uwe Zeisberger","Morten Welinder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"23669","messageId":"E1G0QeX-0003hG-0I@moooo.ath.cx","threadId":"4854","inReplyTo":null,"subject":"git-daemon problem","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-11T22:24:24Z","receivedAt":"2006-07-11T22:24:24Z","isPatch":false,"sender":{"key":"matled@gmx.net","avatar":null},"body":"A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with\ngit-daemon. I started git-daemon on a terminal but did not redirect\nstdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd\nwithout -f but just disowning the process and closing the terminal\nworks fine too, nothing freebsd/daemon(8) specific). After closing\nthe terminal I was not able to use the git-daemon anymore with some\nversions of the git. So now I took some time and tried to find what\nwas the reason for that.\n\nIt seems to be related to the client version too (git without version\nappendix is the current next (028cfcba78c3e4).\n\n583b7ea31b7c16~1 (last good):\n$ git clone git://host:9419/foo\n$ git1.3.2 clone git://host:9419/foo.git\n(cloned successfully, both no output)\n\n583b7ea31b7c16 (first bad):\n$ git clone git://host:9420/foo\nGenerating pack...\nDone counting 6 objects.\nDeltifying 6 objects.\n 100% (6/6) done\n Total 6, written 6 (delta 0), reused 0 (delta 0)\n$ git1.3.2 clone git://host:9420/foo.git\nfatal: cannot mmap packfile '/somewhere/foo/.git/objects/pack/tmp-VX82qz': Invalid argument\nerror: git-fetch-pack: unable to read from git-index-pack\nerror: git-index-pack died with error code 128\nfetch-pack from 'git://host:9420/foo.git' failed.\n[1] 13267 exit 1 git1.3.2 clone git://host:9420/foo.git\n(/somewhere is the cwd on the client)\n\nI tried to find which part of the patch caused the problem and came\nout with the patch below. With this I can clone with git1.3.2 again\nbut then git 1.4.x does not show any statistics about packing, its\njust a starting point to look at. Perhaps someone has an idea why\nthis happens. I've got to sleep now :)\n\n---\n upload-pack.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7b86f69..94f0d85 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -249,7 +249,7 @@ static void create_pack_file(void)\n \t\t\t\tsz = read(pe_pipe[0], progress,\n \t\t\t\t\t sizeof(progress));\n \t\t\t\tif (0 < sz)\n-\t\t\t\t\tsend_client_data(2, progress, sz);\n+\t\t\t\t\twrite(2, progress, sz);\n \t\t\t\telse if (sz == 0) {\n \t\t\t\t\tclose(pe_pipe[0]);\n \t\t\t\t\tpe_pipe[0] = -1;\n"},{"id":"23674","messageId":"7vac7f3fr0.fsf@assigned-by-dhcp.cox.net","threadId":"4854","inReplyTo":"E1G0QeX-0003hG-0I@moooo.ath.cx","subject":"Re: git-daemon problem","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-11T23:04:35Z","receivedAt":"2006-07-11T23:04:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Lederhofer writes:\n\n> A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with\n> git-daemon. I started git-daemon on a terminal but did not redirect\n> stdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd\n> without -f but just disowning the process and closing the terminal\n> works fine too, nothing freebsd/daemon(8) specific). After closing\n> the terminal I was not able to use the git-daemon anymore with some\n> versions of the git. So now I took some time and tried to find what\n> was the reason for that.\n>\n> It seems to be related to the client version too (git without version\n> appendix is the current next (028cfcba78c3e4).\n>\n> 583b7ea31b7c16~1 (last good):\n> $ git clone git://host:9419/foo\n> $ git1.3.2 clone git://host:9419/foo.git\n> (cloned successfully, both no output)\n>\n> 583b7ea31b7c16 (first bad):\n> $ git clone git://host:9420/foo\n> Generating pack...\n> Done counting 6 objects.\n> Deltifying 6 objects.\n> 100% (6/6) done\n> Total 6, written 6 (delta 0), reused 0 (delta 0)\n> $ git1.3.2 clone git://host:9420/foo.git\n> fatal: cannot mmap packfile '/somewhere/foo/.git/objects/pack/tmp-VX82qz': Invalid argument\n> error: git-fetch-pack: unable to read from git-index-pack\n> error: git-index-pack died with error code 128\n> fetch-pack from 'git://host:9420/foo.git' failed.\n> [1] 13267 exit 1 git1.3.2 clone git://host:9420/foo.git\n> (/somewhere is the cwd on the client)\n>\n> I tried to find which part of the patch caused the problem and came\n> out with the patch below. With this I can clone with git1.3.2 again\n> but then git 1.4.x does not show any statistics about packing, its\n> just a starting point to look at. Perhaps someone has an idea why\n> this happens. I've got to sleep now :)\n>\n> ---\n> upload-pack.c | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 7b86f69..94f0d85 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -249,7 +249,7 @@ static void create_pack_file(void)\n> \t\t\t\tsz = read(pe_pipe[0], progress,\n> \t\t\t\t\t sizeof(progress));\n> \t\t\t\tif (0 < sz)\n> -\t\t\t\t\tsend_client_data(2, progress, sz);\n> +\t\t\t\t\twrite(2, progress, sz);\n> \t\t\t\telse if (sz == 0) {\n> \t\t\t\t\tclose(pe_pipe[0]);\n> \t\t\t\t\tpe_pipe[0] = -1;\n\nThis breaks the newer clients that knows how to do side-band\ndoesn't it?\n"},{"id":"23675","messageId":"7v3bd73egd.fsf@assigned-by-dhcp.cox.net","threadId":"4854","inReplyTo":"E1G0QeX-0003hG-0I@moooo.ath.cx","subject":"Re: git-daemon problem","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-11T23:32:34Z","receivedAt":"2006-07-11T23:32:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Lederhofer writes:\n\n> A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with\n> git-daemon. I started git-daemon on a terminal but did not redirect\n> stdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd\n> without -f but just disowning the process and closing the terminal\n> works fine too, nothing freebsd/daemon(8) specific).\n\nThis is because the server side closes fd #2 in such a setup,\nand we still wrote using safe_write() into it. Thanks for\nspotting.\n\nWould this replacement patch help?\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b18eb9b..44038d3 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -51,6 +51,13 @@ static ssize_t send_client_data(int fd, \n \t\tif (fd == 3)\n \t\t\t/* emergency quit */\n \t\t\tfd = 2;\n+\t\tif (fd == 2) {\n+\t\t\t/* people sometomes close fd 2 on the server\n+\t\t\t * side -- making safe_write() to barf.\n+\t\t\t */\n+\t\t\twrite(2, data, sz);\n+\t\t\treturn sz;\n+\t\t}\n \t\treturn safe_write(fd, data, sz);\n \t}\n \tp = data;\n"},{"id":"23689","messageId":"7vlkqz1lja.fsf@assigned-by-dhcp.cox.net","threadId":"4854","inReplyTo":"E1G0QeX-0003hG-0I@moooo.ath.cx","subject":"Re: git-daemon problem","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-12T04:42:33Z","receivedAt":"2006-07-12T04:42:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Lederhofer writes:\n\n> A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with\n> git-daemon. I started git-daemon on a terminal but did not redirect\n> stdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd\n> without -f but just disowning the process and closing the terminal\n> works fine too, nothing freebsd/daemon(8) specific). After closing\n> the terminal I was not able to use the git-daemon anymore with some\n> versions of the git. So now I took some time and tried to find what\n> was the reason for that.\n\nHMMMMMMMMM.\n\nI did this silly experiment. Close, instead of connecting them\nto /dev/null, the low 3 file descriptors:\n\n$ git-daemon 0<&- 1>&- 2>&- --export-all /pub/git &\n$ lsof -p $! | sed -n -e '/ FD /p' -e '/[0-9]u/p'\nCOMMAND PID USER FD TYPE DEVICE SIZE NODE NAME\ngit-daemo 13921 junio 0u IPv6 1973673 TCP *:9419 (LISTEN)\ngit-daemo 13921 junio 1u IPv4 1973674 TCP *:9419 (LISTEN)\n\nIt gets worse. This is what happens when you close only fd #2.\n\n$ git-daemon 2>&- --export-all /pub/git &\n$ lsof -p $! | sed -n -e '/ FD /p' -e '/[0-9]u/p'\nCOMMAND PID USER FD TYPE DEVICE SIZE NODE NAME\ngit-daemo 13961 junio 0u CHR 136,7 9 /dev/pts/7\ngit-daemo 13961 junio 1u CHR 136,7 9 /dev/pts/7\ngit-daemo 13961 junio 2u IPv6 1975187 TCP *:9419 (LISTEN)\ngit-daemo 13961 junio 3u IPv4 1975188 TCP *:9419 (LISTEN)\n\nNow, after we do accept(), we spawn a subprocess in handle(),\nand in the child process dup2() the fd connected to the peer to\nfd 0 and 1 of the child process -- and we do not do anything to\nfd 2 of the child process.\n\nIf you do not close any of the low 3 file descriptors, fd 2 of\nthe child process is connected to whatever error stream of\ndaemon is, so you would not see this problem, but this certainly\nis bad.\n\nMaybe we should check if fd 2 is sane at daemon startup, and\notherwise open /dev/null for writing and dup2 it to fd 2?\n\nCurrently, under --inetd mode we have freopen of stderr, but\nthat does not help this issue. It would make die() and error()\nin daemon itself behave sanely but when you start the daemon\nwith the low file descriptors closed, fileno(stderr) may be\ndifferent from 2.\n"},{"id":"23723","messageId":"E1G0kNV-0006LK-QW@moooo.ath.cx","threadId":"4854","inReplyTo":"7vlkqz1lja.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-daemon problem","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-12T19:28:09Z","receivedAt":"2006-07-12T19:28:09Z","isPatch":false,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Junio C Hamano wrote:\n> This breaks the newer clients that knows how to do side-band\n> doesn't it?\nProbably, this patch is just to give a starting point where the\nproblem could be.\n\n> Would this replacement patch help?\n[use write instead of safe_write]\n\nThis seems to fix it. Perhaps it should be xwrite instead of write.\n\n> Maybe we should check if fd 2 is sane at daemon startup, and\n> otherwise open /dev/null for writing and dup2 it to fd 2?\ndaemon startup is probably not the right place because as long as the\nterminal is open this will be fine.\n"},{"id":"23743","messageId":"7vveq2ukho.fsf@assigned-by-dhcp.cox.net","threadId":"4854","inReplyTo":"E1G0kNV-0006LK-QW@moooo.ath.cx","subject":"Re: git-daemon problem","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-13T05:44:35Z","receivedAt":"2006-07-13T05:44:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Lederhofer writes:\n\n> Junio C Hamano wrote:\n>> This breaks the newer clients that knows how to do side-band\n>> doesn't it?\n> Probably, this patch is just to give a starting point where the\n> problem could be.\n>\n>> Would this replacement patch help?\n> [use write instead of safe_write]\n>\n> This seems to fix it. Perhaps it should be xwrite instead of write.\n>\n>> Maybe we should check if fd 2 is sane at daemon startup, and\n>> otherwise open /dev/null for writing and dup2 it to fd 2?\n> daemon startup is probably not the right place because as long as the\n> terminal is open this will be fine.\n\nHmph. In the part of my message you did not quote:\n\n\t$ git-daemon 0<&- 1>&- 2>&- --export-all /pub/git\n\nleaves listening sockets at fd 0/1 without any fd 2, and\n\n\t$ git-daemon 2>&- --export-all /pub/git\n\nallocates listening socket at FD 2 (because FD 0 and FD 1 are\noccupied).\n\nNow, after we do accept(), we spawn a subprocess in handle(),\nand in the child process dup2() the fd connected to the peer to\nfd 0 and 1 of the child process -- and we do not do anything to\nfd 2 of the child process. So in the latter case, my tentative\npatch would write error message to the listening socket -- ugh.\n\nAnd as you say, fd 2 might be connected to the terminal and\nhealthy when you start the daemon, but later you can close the\nterminal, so there is no sane place for us to try anything\nsensible.\n\nThe only \"right\" solution I could think of is to properly\ndaemonize git-daemon when not running under --inetd mode. Close\nand open /dev/null the low three fds, and dissociate the process\nfrom the controlling terminal (did I forget anything else --\nperhaps chdir(\"/\") at the top?). And we keep the current\nbehaviour of assuming the sane set of low three fds when a new\noption --debug is given to help people look at its stderr. The\ntentative patch to upload-pack would become moot at that point.\n\nHmm?\n"},{"id":"23747","messageId":"E1G0vqW-00077L-1t@moooo.ath.cx","threadId":"4854","inReplyTo":"7vveq2ukho.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-daemon problem","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T07:42:52Z","receivedAt":"2006-07-13T07:42:52Z","isPatch":false,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Junio C Hamano wrote:\n> Matthias Lederhofer writes:\n> > Junio C Hamano wrote:\n> >> Maybe we should check if fd 2 is sane at daemon startup, and\n> >> otherwise open /dev/null for writing and dup2 it to fd 2?\n> > daemon startup is probably not the right place because as long as the\n> > terminal is open this will be fine.\n> \n> Hmph. In the part of my message you did not quote:\n> \n> \t$ git-daemon 0<&- 1>&- 2>&- --export-all /pub/git\n> \n> leaves listening sockets at fd 0/1 without any fd 2, and\n> \n> \t$ git-daemon 2>&- --export-all /pub/git\n> \n> allocates listening socket at FD 2 (because FD 0 and FD 1 are\n> occupied).\n> \n> Now, after we do accept(), we spawn a subprocess in handle(),\n> and in the child process dup2() the fd connected to the peer to\n> fd 0 and 1 of the child process -- and we do not do anything to\n> fd 2 of the child process. So in the latter case, my tentative\n> patch would write error message to the listening socket -- ugh.\n> \n> And as you say, fd 2 might be connected to the terminal and\n> healthy when you start the daemon, but later you can close the\n> terminal, so there is no sane place for us to try anything\n> sensible.\n> \n> The only \"right\" solution I could think of is to properly\n> daemonize git-daemon when not running under --inetd mode. Close\n> and open /dev/null the low three fds, and dissociate the process\n> from the controlling terminal (did I forget anything else --\n> perhaps chdir(\"/\") at the top?). And we keep the current\n> behaviour of assuming the sane set of low three fds when a new\n> option --debug is given to help people look at its stderr. The\n> tentative patch to upload-pack would become moot at that point.\n> \n> Hmm?\n\nNow I see what you mean. Checking the fds at daemon startup should\nwork too (using xwrite/write instead of safe_write).\n\nIs daemon(3) portable? Otherwise it should be:\nfork(); setsid(); chdir(\"/\"); open(\"/dev/null\", O_RDWR, 0); dup2(..);\n(looking at the daemon(3) implementations of glibc and freebsd libc)\nThe freebsd implementation of daemon(3) ignores SIGHUP for fork() and\nsetsid() with the comment \"A SIGHUP may be thrown when the parent\nexits below.\"\n\nWith the option to run in background (or the other way around -- not\nto run in background) should also come an option to write a pid file.\n"},{"id":"23754","messageId":"E1G0znB-0002IH-5K@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0zj7-0001c1-8q@moooo.ath.cx","subject":"[PATCH 1/5] daemon: use a custom die routine with syslog","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T10:02:29Z","receivedAt":"2006-07-13T10:02:29Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Removed the git-daemon prefix from die() because no other call to die\ndoes this.\n\nSigned-off-by: Matthias Lederhofer \n---\n daemon.c | 19 +++++++++++--------\n 1 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex e096bd7..a7636bc 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -95,6 +95,12 @@ static void loginfo(const char *err, ...\n \tva_end(params);\n }\n \n+static void NORETURN daemon_die(const char *err, va_list params)\n+{\n+\tlogreport(LOG_ERR, err, params);\n+\texit(1);\n+}\n+\n static int avoid_alias(char *p)\n {\n \tint sl, ndot;\n@@ -746,17 +752,14 @@ int main(int argc, char **argv)\n \t\tusage(daemon_usage);\n \t}\n \n-\tif (log_syslog)\n+\tif (log_syslog) {\n \t\topenlog(\"git-daemon\", 0, LOG_DAEMON);\n-\n-\tif (strict_paths && (!ok_paths || !*ok_paths)) {\n-\t\tif (!inetd_mode)\n-\t\t\tdie(\"git-daemon: option --strict-paths requires a whitelist\");\n-\n-\t\tlogerror(\"option --strict-paths requires a whitelist\");\n-\t\texit (1);\n+\t\tset_die_routine(daemon_die);\n \t}\n \n+\tif (strict_paths && (!ok_paths || !*ok_paths))\n+\t\tdie(\"option --strict-paths requires a whitelist\");\n+\n \tif (inetd_mode) {\n \t\tstruct sockaddr_storage ss;\n \t\tstruct sockaddr *peer = (struct sockaddr *)&ss;\n-- \n1.4.1.gb16f\n"},{"id":"23757","messageId":"E1G0znB-0002IO-61@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0zj7-0001c1-8q@moooo.ath.cx","subject":"[PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T10:10:51Z","receivedAt":"2006-07-13T10:10:51Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Signed-off-by: Matthias Lederhofer \n---\n daemon.c | 20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex a7636bc..e6b1730 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -662,6 +662,24 @@ static int service_loop(int socknum, int\n \t}\n }\n \n+/* if any standard file descriptor is missing open it to /dev/null */\n+static void sanitize_stdfds(void)\n+{\n+\tint devnull = -1, i;\n+\tstruct stat buf;\n+\tfor (i = 0; i < 3; ++i) {\n+\t\tif (fstat(i, &buf) != -1)\n+\t\t\tcontinue;\n+\t\tif (devnull == -1 &&\n+\t\t\t(devnull = open(\"/dev/null\", O_RDWR, 0)) == -1)\n+\t\t\tdie(\"open /dev/null failed: %s\", strerror(errno));\n+\t\tif (dup2(devnull, i) != i)\n+\t\t\tdie(\"dup2 failed: %s\", strerror(errno));\n+\t}\n+\tif (devnull != -1)\n+\t\tclose(devnull);\n+}\n+\n static int serve(int port)\n {\n \tint socknum, *socklist;\n@@ -773,5 +791,7 @@ int main(int argc, char **argv)\n \t\treturn execute(peer);\n \t}\n \n+\tsanitize_stdfds();\n+\n \treturn serve(port);\n }\n-- \n1.4.1.gb16f\n"},{"id":"23755","messageId":"E1G0znB-0002IZ-7e@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0zj7-0001c1-8q@moooo.ath.cx","subject":"[PATCH 4/5] daemon: new option --pid-file= to store the pid","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T10:18:08Z","receivedAt":"2006-07-13T10:18:08Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Signed-off-by: Matthias Lederhofer \n---\n daemon.c | 17 +++++++++++++++++\n 1 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex e6b1730..4b85930 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -680,6 +680,15 @@ static void sanitize_stdfds(void)\n \t\tclose(devnull);\n }\n \n+static void store_pid(const char *path)\n+{\n+\tFILE *f = fopen(path, \"w\");\n+\tif (!f)\n+\t\tdie(\"cannot open pid file %s: %s\", path, strerror(errno));\n+\tfprintf(f, \"%d\\n\", getpid());\n+\tfclose(f);\n+}\n+\n static int serve(int port)\n {\n \tint socknum, *socklist;\n@@ -695,6 +704,7 @@ int main(int argc, char **argv)\n {\n \tint port = DEFAULT_GIT_PORT;\n \tint inetd_mode = 0;\n+\tconst char *pid_file = NULL;\n \tint i;\n \n \t/* Without this we cannot rely on waitpid() to tell\n@@ -759,6 +769,10 @@ int main(int argc, char **argv)\n \t\t\tuser_path = arg + 12;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strncmp(arg, \"--pid-file=\", 11)) {\n+\t\t\tpid_file = arg + 11;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\tok_paths = &argv[i+1];\n \t\t\tbreak;\n@@ -793,5 +807,8 @@ int main(int argc, char **argv)\n \n \tsanitize_stdfds();\n \n+\tif (pid_file)\n+\t\tstore_pid(pid_file);\n+\n \treturn serve(port);\n }\n-- \n1.4.1.gb16f\n"},{"id":"23756","messageId":"E1G0znB-0002If-8A@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0zj7-0001c1-8q@moooo.ath.cx","subject":"[PATCH 5/5] daemon: new option --detach to run git-daemon in background","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T10:32:31Z","receivedAt":"2006-07-13T10:32:31Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Signed-off-by: Matthias Lederhofer \n---\n daemon.c | 32 +++++++++++++++++++++++++++++++-\n 1 files changed, 31 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 4b85930..9f4bc20 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -662,6 +662,27 @@ static int service_loop(int socknum, int\n \t}\n }\n \n+static void daemonize(void)\n+{\n+\tint devnull = -1;\n+\tswitch (fork()) {\n+\t\tcase 0:\n+\t\t\tbreak;\n+\t\tcase -1:\n+\t\t\tdie(\"fork failed: %s\", strerror(errno));\n+\t\tdefault:\n+\t\t\texit(0);\n+\t}\n+\tif (setsid() == -1)\n+\t\tdie(\"setsid failed: %s\", strerror(errno));\n+\tif ((devnull = open(\"/dev/null\", O_RDWR, 0)) == -1)\n+\t\tdie(\"open /dev/null failed: %s\", strerror(errno));\n+\tif (dup2(devnull, 0) != 0 ||\n+\t\tdup2(devnull, 1) != 1 ||\n+\t\tdup2(devnull, 2) != 2)\n+\t\tdie(\"dup2 failed: %s\", strerror(errno));\n+}\n+\n /* if any standard file descriptor is missing open it to /dev/null */\n static void sanitize_stdfds(void)\n {\n@@ -705,6 +726,7 @@ int main(int argc, char **argv)\n \tint port = DEFAULT_GIT_PORT;\n \tint inetd_mode = 0;\n \tconst char *pid_file = NULL;\n+\tint detach = 0;\n \tint i;\n \n \t/* Without this we cannot rely on waitpid() to tell\n@@ -773,6 +795,11 @@ int main(int argc, char **argv)\n \t\t\tpid_file = arg + 11;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--detach\")) {\n+\t\t\tdetach = 1;\n+\t\t\tlog_syslog = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\tok_paths = &argv[i+1];\n \t\t\tbreak;\n@@ -805,7 +832,10 @@ int main(int argc, char **argv)\n \t\treturn execute(peer);\n \t}\n \n-\tsanitize_stdfds();\n+\tif (detach)\n+\t\tdaemonize();\n+\telse\n+\t\tsanitize_stdfds();\n \n \tif (pid_file)\n \t\tstore_pid(pid_file);\n-- \n1.4.1.gb16f\n"},{"id":"23758","messageId":"E1G0znB-0002IT-77@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0zj7-0001c1-8q@moooo.ath.cx","subject":"[PATCH 3/5] upload-pack: ignore write errors to stderr","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T11:07:59Z","receivedAt":"2006-07-13T11:07:59Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Signed-off-by: Matthias Lederhofer \n---\n upload-pack.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b18eb9b..94aa0da 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -51,6 +51,10 @@ static ssize_t send_client_data(int fd, \n \t\tif (fd == 3)\n \t\t\t/* emergency quit */\n \t\t\tfd = 2;\n+\t\tif (fd == 2) {\n+\t\t\txwrite(fd, data, sz);\n+\t\t\treturn sz;\n+\t\t}\n \t\treturn safe_write(fd, data, sz);\n \t}\n \tp = data;\n-- \n1.4.1.gb16f\n"},{"id":"23752","messageId":"20060713114221.GU11245@skl-net.de","threadId":"4854","inReplyTo":"7vveq2ukho.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-daemon problem","fromName":"Andre Noll","fromEmail":"maan@systemlinux.org","sentAt":"2006-07-13T11:42:21Z","receivedAt":"2006-07-13T11:42:21Z","isPatch":false,"sender":{"key":"maan@systemlinux.org","avatar":null},"body":"On 22:44, Junio C Hamano wrote:\n\n> The only \"right\" solution I could think of is to properly\n> daemonize git-daemon when not running under --inetd mode. Close\n> and open /dev/null the low three fds, and dissociate the process\n> from the controlling terminal (did I forget anything else --\n> perhaps chdir(\"/\") at the top?). And we keep the current\n> behaviour of assuming the sane set of low three fds when a new\n> option --debug is given to help people look at its stderr. The\n> tentative patch to upload-pack would become moot at that point.\n> \n> Hmm?\n\nA common solution for this problem is \n\n\twhile (1) {\n\t\tint fd;\n\n\t\tfd = open(\"/dev/null\", O_RDWR);\n\t\tif (fd < 0)\n\t\t\texit(EX_OSERR);\n\t\tif (fd > 2) {\n\t\t\tclose(fd);\n\t\t\tbreak;\n\t\t}\n\t}\n\nSee\n\n\thttp://rechner.lst.de/~okir/blackhats/node41.html\n\nAndre\n-- \nThe only person who always got his work done by Friday was Robinson Crusoe\n"},{"id":"23753","messageId":"E1G0zj7-0001c1-8q@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0QeX-0003hG-0I@moooo.ath.cx","subject":"Re: git-daemon problem","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T11:51:29Z","receivedAt":"2006-07-13T11:51:29Z","isPatch":false,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Here are some patches that should solve this.\nNote: The first patch is not really related to this problem but I\nthink the die error message should go to syslog when --syslog was\nused. (And I did not have to add more if-clauses.)\n\nDocumentation will follow if the changes are ok.\n"},{"id":"23761","messageId":"20060713152725.7a5081df.froese@gmx.de","threadId":"4854","inReplyTo":"E1G0znB-0002IO-61@moooo.ath.cx","subject":"Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null","fromName":"Edgar Toernig","fromEmail":"froese@gmx.de","sentAt":"2006-07-13T13:27:25Z","receivedAt":"2006-07-13T13:27:25Z","isPatch":true,"sender":{"key":"froese@gmx.de","avatar":null},"body":"Matthias Lederhofer wrote:\n>\n> +/* if any standard file descriptor is missing open it to /dev/null */\n> +static void sanitize_stdfds(void)\n> +{\n> +\tint devnull = -1, i;\n> +\tstruct stat buf;\n> +\tfor (i = 0; i < 3; ++i) {\n> +\t\tif (fstat(i, &buf) != -1)\n> +\t\t\tcontinue;\n> +\t\tif (devnull == -1 &&\n> +\t\t\t(devnull = open(\"/dev/null\", O_RDWR, 0)) == -1)\n> +\t\t\tdie(\"open /dev/null failed: %s\", strerror(errno));\n> +\t\tif (dup2(devnull, i) != i)\n> +\t\t\tdie(\"dup2 failed: %s\", strerror(errno));\n> +\t}\n> +\tif (devnull != -1)\n> +\t\tclose(devnull);\n> +}\n\nThis looks broken. The open will return i as this is\nthe lowest free fd. I don't know what POSIX says\nabout dup2(i,i) but anyway, you close it at the end\nwhich completely defeats the intent of the function.\n\nHow's this?\n\n\tdevnull = open(\"/dev/null\", O_RDWR, 0);\n\tif (devnull == 0)\n\t\tdevnull = dup(devnull);\n\tif (devnull == 1)\n\t\tdevnull = dup(devnull);\n\tif (devnull == -1)\n\t\tdie(\"open/dup /dev/null failed: %s\", strerror(errno));\n\tif (devnull > 2)\n\t\tclose(devnull);\n\nCiao, ET.\n"},{"id":"23762","messageId":"20060713153703.05f862ee.froese@gmx.de","threadId":"4854","inReplyTo":"E1G0znB-0002If-8A@moooo.ath.cx","subject":"Re: [PATCH 5/5] daemon: new option --detach to run git-daemon in background","fromName":"Edgar Toernig","fromEmail":"froese@gmx.de","sentAt":"2006-07-13T13:37:03Z","receivedAt":"2006-07-13T13:37:03Z","isPatch":true,"sender":{"key":"froese@gmx.de","avatar":null},"body":"Matthias Lederhofer wrote:\n>\n> [daemonize]\n> +\tif ((devnull = open(\"/dev/null\", O_RDWR, 0)) == -1)\n> +\t\tdie(\"open /dev/null failed: %s\", strerror(errno));\n> +\tif (dup2(devnull, 0) != 0 ||\n> +\t\tdup2(devnull, 1) != 1 ||\n> +\t\tdup2(devnull, 2) != 2)\n> +\t\tdie(\"dup2 failed: %s\", strerror(errno));\n> +}\n\nHmm... leaks devnull. Why not simply close(0/1/2) and\nlet sanitize_stdfds take care of the rest?\n\nCiao, ET.\n"},{"id":"23763","messageId":"E1G11nq-00076g-Aa@moooo.ath.cx","threadId":"4854","inReplyTo":"20060713152725.7a5081df.froese@gmx.de","subject":"Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T14:04:30Z","receivedAt":"2006-07-13T14:04:30Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Edgar Toernig wrote:\n> Matthias Lederhofer wrote:\n> >\n> > +/* if any standard file descriptor is missing open it to /dev/null */\n> > +static void sanitize_stdfds(void)\n> > +{\n> > +\tint devnull = -1, i;\n> > +\tstruct stat buf;\n> > +\tfor (i = 0; i < 3; ++i) {\n> > +\t\tif (fstat(i, &buf) != -1)\n> > +\t\t\tcontinue;\n> > +\t\tif (devnull == -1 &&\n> > +\t\t\t(devnull = open(\"/dev/null\", O_RDWR, 0)) == -1)\n> > +\t\t\tdie(\"open /dev/null failed: %s\", strerror(errno));\n> > +\t\tif (dup2(devnull, i) != i)\n> > +\t\t\tdie(\"dup2 failed: %s\", strerror(errno));\n> > +\t}\n> > +\tif (devnull != -1)\n> > +\t\tclose(devnull);\n> > +}\n> \n> This looks broken. The open will return i as this is\n> the lowest free fd. I don't know what POSIX says\n> about dup2(i,i) but anyway, you close it at the end\n> which completely defeats the intent of the function.\n> \n> How's this?\n> \n> \tdevnull = open(\"/dev/null\", O_RDWR, 0);\n> \tif (devnull == 0)\n> \t\tdevnull = dup(devnull);\n> \tif (devnull == 1)\n> \t\tdevnull = dup(devnull);\n> \tif (devnull == -1)\n> \t\tdie(\"open/dup /dev/null failed: %s\", strerror(errno));\n> \tif (devnull > 2)\n> \t\tclose(devnull);\n\nYou're right (also for the daemonize function to use sanitize_stdfds).\nThe code looks good to me, this could also be done using a while-loop\n(making it a little bit shorter, I don't know what is easier to read):\n\n devnull = open(\"/dev/null\", O_RDWR, 0);\n while (devnull != -1 && devnull < 2)\n dup(devnull);\n if (devnull == -1)\n die(\"..\");\n close(devnull);\n\n(This is similar to what Andre Noll posted.)\n\nI'll correct and resend those patches later.\n"},{"id":"23764","messageId":"20060713143649.GA3395@informatik.uni-freiburg.de","threadId":"4854","inReplyTo":"E1G11nq-00076g-Aa@moooo.ath.cx","subject":"Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null","fromName":"Uwe Zeisberger","fromEmail":"zeisberg@informatik.uni-freiburg.de","sentAt":"2006-07-13T14:36:49Z","receivedAt":"2006-07-13T14:36:49Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello Matthias,\n\n(Do you know you set the Mail-Followup-To Header? That is annoying.)\n\n> devnull = open(\"/dev/null\", O_RDWR, 0);\n> while (devnull != -1 && devnull < 2)\n> dup(devnull);\nYou mean\n\n\t devnull = dup(devnull);\n\n, don't you?\n\n> if (devnull == -1)\n> die(\"..\");\n> close(devnull);\n\nBest regards\nUwe\n\n-- \nUwe Zeisberger\n\nprimes where sieve (p:xs) = [ x | x<-xs, x `rem` p /= 0 ]; \\\nprimes = map head (iterate sieve [2..])\n"},{"id":"23765","messageId":"118833cc0607130837u30c58d53lc785f56d45ef970c@mail.gmail.com","threadId":"4854","inReplyTo":"E1G0znB-0002IO-61@moooo.ath.cx","subject":"Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2006-07-13T15:37:50Z","receivedAt":"2006-07-13T15:37:50Z","isPatch":true,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":"> + if (devnull == -1 &&\n> + (devnull = open(\"/dev/null\", O_RDWR, 0)) == -1)\n> + die(\"open /dev/null failed: %s\", strerror(errno));\n> + if (dup2(devnull, i) != i)\n> + die(\"dup2 failed: %s\", strerror(errno));\n\n\"die\" probably won't work well at this point.\n\nShould git (and most other programs) do something like this in general?\nfprintf will happily write to fd=2 regardless of whether that is some critical\nfile you opened.\n\nMorten\n"},{"id":"23766","messageId":"E1G13ew-0005Iv-ST@moooo.ath.cx","threadId":"4854","inReplyTo":"118833cc0607130837u30c58d53lc785f56d45ef970c@mail.gmail.com","subject":"Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T16:03:26Z","receivedAt":"2006-07-13T16:03:26Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Morten Welinder wrote:\n> >+ if (devnull == -1 &&\n> >+ (devnull = open(\"/dev/null\", O_RDWR, 0)) == -1)\n> >+ die(\"open /dev/null failed: %s\", strerror(errno));\n> >+ if (dup2(devnull, i) != i)\n> >+ die(\"dup2 failed: %s\", strerror(errno));\n> \n> \"die\" probably won't work well at this point.\nAt least with --syslog there will be an error message in the logs.\nIf the user does not use --syslog and closes fd 2 it is just his own\nfault imho.\n\n> Should git (and most other programs) do something like this in general?\n> fprintf will happily write to fd=2 regardless of whether that is some critical\n> file you opened.\nI thought of that too. It might be not that important because I\ncannot think of anyway that this could happen accidentally or could be\nexploited.\n"},{"id":"23769","messageId":"E1G146l-0006jI-G8@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0znB-0002IO-61@moooo.ath.cx","subject":"[PATCH 2.1/5] daemon: if one of the standard fds is missing open it to /dev/null","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T16:32:11Z","receivedAt":"2006-07-13T16:32:11Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Signed-off-by: Matthias Lederhofer \n---\nfixed sanitize_stdfds\n---\n daemon.c | 14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex a7636bc..01ccda3 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -662,6 +662,18 @@ static int service_loop(int socknum, int\n \t}\n }\n \n+/* if any standard file descriptor is missing open it to /dev/null */\n+static void sanitize_stdfds(void)\n+{\n+\tint fd = open(\"/dev/null\", O_RDWR, 0);\n+\twhile (fd != -1 && fd < 2)\n+\t\tfd = dup(fd);\n+\tif (fd == -1)\n+\t\tdie(\"open /dev/null or dup failed: %s\", strerror(errno));\n+\tif (fd > 2)\n+\t\tclose(fd);\n+}\n+\n static int serve(int port)\n {\n \tint socknum, *socklist;\n@@ -773,5 +785,7 @@ int main(int argc, char **argv)\n \t\treturn execute(peer);\n \t}\n \n+\tsanitize_stdfds();\n+\n \treturn serve(port);\n }\n-- \n1.4.1.g8b4b\n"},{"id":"23770","messageId":"E1G14LJ-0007q1-Fa@moooo.ath.cx","threadId":"4854","inReplyTo":"20060713153703.05f862ee.froese@gmx.de","subject":"Re: [PATCH 5.1/5] daemon: new option --detach to run git-daemon in background","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-13T16:47:13Z","receivedAt":"2006-07-13T16:47:13Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Signed-off-by: Matthias Lederhofer \n---\nEdgar Toernig wrote:\n> Hmm... leaks devnull. Why not simply close(0/1/2) and\n> let sanitize_stdfds take care of the rest?\n---\n daemon.c | 29 ++++++++++++++++++++++++++++-\n 1 files changed, 28 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex cdc4266..e4ec676 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -674,6 +674,24 @@ static void sanitize_stdfds(void)\n \t\tclose(fd);\n }\n \n+static void daemonize(void)\n+{\n+\tswitch (fork()) {\n+\t\tcase 0:\n+\t\t\tbreak;\n+\t\tcase -1:\n+\t\t\tdie(\"fork failed: %s\", strerror(errno));\n+\t\tdefault:\n+\t\t\texit(0);\n+\t}\n+\tif (setsid() == -1)\n+\t\tdie(\"setsid failed: %s\", strerror(errno));\n+\tclose(0);\n+\tclose(1);\n+\tclose(2);\n+\tsanitize_stdfds();\n+}\n+\n static void store_pid(const char *path)\n {\n \tFILE *f = fopen(path, \"w\");\n@@ -699,6 +717,7 @@ int main(int argc, char **argv)\n \tint port = DEFAULT_GIT_PORT;\n \tint inetd_mode = 0;\n \tconst char *pid_file = NULL;\n+\tint detach = 0;\n \tint i;\n \n \t/* Without this we cannot rely on waitpid() to tell\n@@ -767,6 +786,11 @@ int main(int argc, char **argv)\n \t\t\tpid_file = arg + 11;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--detach\")) {\n+\t\t\tdetach = 1;\n+\t\t\tlog_syslog = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\tok_paths = &argv[i+1];\n \t\t\tbreak;\n@@ -799,7 +823,10 @@ int main(int argc, char **argv)\n \t\treturn execute(peer);\n \t}\n \n-\tsanitize_stdfds();\n+\tif (detach)\n+\t\tdaemonize();\n+\telse\n+\t\tsanitize_stdfds();\n \n \tif (pid_file)\n \t\tstore_pid(pid_file);\n-- \n1.4.1.g8b4b\n"},{"id":"23820","messageId":"E1G1Pyr-0004uv-JX@moooo.ath.cx","threadId":"4854","inReplyTo":"E1G0zj7-0001c1-8q@moooo.ath.cx","subject":"[PATCH] daemon: documentation for --reuseaddr, --detach and --pid-file","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-07-14T15:53:29Z","receivedAt":"2006-07-14T15:53:29Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Signed-off-by: Matthias Lederhofer \n---\nMatthias Lederhofer wrote:\n> Documentation will follow if the changes are ok.\nHere it is. I just found that --reuseaddr is not documented yet too\n(I could really have used that while testing the git-daemon\npatches...) but I have no idea how to describe it for someone who\ndoes not know what it means. Perhaps someone else has an idea.\n---\n Documentation/git-daemon.txt | 8 +++++++-\n daemon.c | 2 +-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 4c357da..f5b08a6 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git-daemon' [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n [--timeout=n] [--init-timeout=n] [--strict-paths]\n [--base-path=path] [--user-path | --user-path=path]\n-\t [directory...]\n+\t [--reuseaddr] [--detach] [--pid-file=file] [directory...]\n \n DESCRIPTION\n -----------\n@@ -82,6 +82,12 @@ OPTIONS\n --verbose::\n \tLog details about the incoming connections and requested files.\n \n+--detach::\n+\tDetach from the shell. Implies --syslog.\n+\n+--pid-file=file::\n+\tSave the process id in 'file'.\n+\n ::\n \tA directory to add to the whitelist of allowed directories. Unless\n \t--strict-paths is specified this will also include subdirectories\ndiff --git a/daemon.c b/daemon.c\nindex e4ec676..810837f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -19,7 +19,7 @@ static const char daemon_usage[] =\n \"git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\\n\"\n \" [--timeout=n] [--init-timeout=n] [--strict-paths]\\n\"\n \" [--base-path=path] [--user-path | --user-path=path]\\n\"\n-\" [--reuseaddr] [directory...]\";\n+\" [--reuseaddr] [--detach] [--pid-file=file] [directory...]\";\n \n /* List of acceptable pathname prefixes */\n static char **ok_paths = NULL;\n-- \n1.4.1.g8b4b\n"}]}