# git-daemon problem 23 messages from 2006-07-11 to 2006-07-14. Participants: Matthias Lederhofer, Junio C Hamano, Andre Noll, Edgar Toernig, Uwe Zeisberger, Morten Welinder. Thread: https://gitlist.dev/t/4854 ## Matthias Lederhofer, 2006-07-11 22:24 Subject: git-daemon problem Message-ID: URL: https://gitlist.dev/e/E1G0QeX-0003hG-0I%40moooo.ath.cx ``` A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with git-daemon. I started git-daemon on a terminal but did not redirect stdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd without -f but just disowning the process and closing the terminal works fine too, nothing freebsd/daemon(8) specific). After closing the terminal I was not able to use the git-daemon anymore with some versions of the git. So now I took some time and tried to find what was the reason for that. It seems to be related to the client version too (git without version appendix is the current next (028cfcba78c3e4). 583b7ea31b7c16~1 (last good): $ git clone git://host:9419/foo $ git1.3.2 clone git://host:9419/foo.git (cloned successfully, both no output) 583b7ea31b7c16 (first bad): $ git clone git://host:9420/foo Generating pack... Done counting 6 objects. Deltifying 6 objects. 100% (6/6) done Total 6, written 6 (delta 0), reused 0 (delta 0) $ git1.3.2 clone git://host:9420/foo.git fatal: cannot mmap packfile '/somewhere/foo/.git/objects/pack/tmp-VX82qz': Invalid argument error: git-fetch-pack: unable to read from git-index-pack error: git-index-pack died with error code 128 fetch-pack from 'git://host:9420/foo.git' failed. [1] 13267 exit 1 git1.3.2 clone git://host:9420/foo.git (/somewhere is the cwd on the client) I tried to find which part of the patch caused the problem and came out with the patch below. With this I can clone with git1.3.2 again but then git 1.4.x does not show any statistics about packing, its just a starting point to look at. Perhaps someone has an idea why this happens. I've got to sleep now :) --- upload-pack.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index 7b86f69..94f0d85 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -249,7 +249,7 @@ static void create_pack_file(void) sz = read(pe_pipe[0], progress, sizeof(progress)); if (0 < sz) - send_client_data(2, progress, sz); + write(2, progress, sz); else if (sz == 0) { close(pe_pipe[0]); pe_pipe[0] = -1; ``` ## Junio C Hamano, 2006-07-11 23:04 Subject: Re: git-daemon problem Message-ID: <7vac7f3fr0.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vac7f3fr0.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Matthias Lederhofer writes: > A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with > git-daemon. I started git-daemon on a terminal but did not redirect > stdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd > without -f but just disowning the process and closing the terminal > works fine too, nothing freebsd/daemon(8) specific). After closing > the terminal I was not able to use the git-daemon anymore with some > versions of the git. So now I took some time and tried to find what > was the reason for that. > > It seems to be related to the client version too (git without version > appendix is the current next (028cfcba78c3e4). > > 583b7ea31b7c16~1 (last good): > $ git clone git://host:9419/foo > $ git1.3.2 clone git://host:9419/foo.git > (cloned successfully, both no output) > > 583b7ea31b7c16 (first bad): > $ git clone git://host:9420/foo > Generating pack... > Done counting 6 objects. > Deltifying 6 objects. > 100% (6/6) done > Total 6, written 6 (delta 0), reused 0 (delta 0) > $ git1.3.2 clone git://host:9420/foo.git > fatal: cannot mmap packfile '/somewhere/foo/.git/objects/pack/tmp-VX82qz': Invalid argument > error: git-fetch-pack: unable to read from git-index-pack > error: git-index-pack died with error code 128 > fetch-pack from 'git://host:9420/foo.git' failed. > [1] 13267 exit 1 git1.3.2 clone git://host:9420/foo.git > (/somewhere is the cwd on the client) > > I tried to find which part of the patch caused the problem and came > out with the patch below. With this I can clone with git1.3.2 again > but then git 1.4.x does not show any statistics about packing, its > just a starting point to look at. Perhaps someone has an idea why > this happens. I've got to sleep now :) > > --- > upload-pack.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/upload-pack.c b/upload-pack.c > index 7b86f69..94f0d85 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -249,7 +249,7 @@ static void create_pack_file(void) > sz = read(pe_pipe[0], progress, > sizeof(progress)); > if (0 < sz) > - send_client_data(2, progress, sz); > + write(2, progress, sz); > else if (sz == 0) { > close(pe_pipe[0]); > pe_pipe[0] = -1; This breaks the newer clients that knows how to do side-band doesn't it? ``` ## Junio C Hamano, 2006-07-11 23:32 Subject: Re: git-daemon problem Message-ID: <7v3bd73egd.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v3bd73egd.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Matthias Lederhofer writes: > A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with > git-daemon. I started git-daemon on a terminal but did not redirect > stdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd > without -f but just disowning the process and closing the terminal > works fine too, nothing freebsd/daemon(8) specific). This is because the server side closes fd #2 in such a setup, and we still wrote using safe_write() into it. Thanks for spotting. Would this replacement patch help? diff --git a/upload-pack.c b/upload-pack.c index b18eb9b..44038d3 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -51,6 +51,13 @@ static ssize_t send_client_data(int fd, if (fd == 3) /* emergency quit */ fd = 2; + if (fd == 2) { + /* people sometomes close fd 2 on the server + * side -- making safe_write() to barf. + */ + write(2, data, sz); + return sz; + } return safe_write(fd, data, sz); } p = data; ``` ## Junio C Hamano, 2006-07-12 04:42 Subject: Re: git-daemon problem Message-ID: <7vlkqz1lja.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vlkqz1lja.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Matthias Lederhofer writes: > A few weeks ago upgrading from 1.3.x to 1.4.1 I had a problem with > git-daemon. I started git-daemon on a terminal but did not redirect > stdin/stdout/stderr to /dev/null (actually using daemon(8) on freebsd > without -f but just disowning the process and closing the terminal > works fine too, nothing freebsd/daemon(8) specific). After closing > the terminal I was not able to use the git-daemon anymore with some > versions of the git. So now I took some time and tried to find what > was the reason for that. HMMMMMMMMM. I did this silly experiment. Close, instead of connecting them to /dev/null, the low 3 file descriptors: $ git-daemon 0<&- 1>&- 2>&- --export-all /pub/git & $ lsof -p $! | sed -n -e '/ FD /p' -e '/[0-9]u/p' COMMAND PID USER FD TYPE DEVICE SIZE NODE NAME git-daemo 13921 junio 0u IPv6 1973673 TCP *:9419 (LISTEN) git-daemo 13921 junio 1u IPv4 1973674 TCP *:9419 (LISTEN) It gets worse. This is what happens when you close only fd #2. $ git-daemon 2>&- --export-all /pub/git & $ lsof -p $! | sed -n -e '/ FD /p' -e '/[0-9]u/p' COMMAND PID USER FD TYPE DEVICE SIZE NODE NAME git-daemo 13961 junio 0u CHR 136,7 9 /dev/pts/7 git-daemo 13961 junio 1u CHR 136,7 9 /dev/pts/7 git-daemo 13961 junio 2u IPv6 1975187 TCP *:9419 (LISTEN) git-daemo 13961 junio 3u IPv4 1975188 TCP *:9419 (LISTEN) Now, after we do accept(), we spawn a subprocess in handle(), and in the child process dup2() the fd connected to the peer to fd 0 and 1 of the child process -- and we do not do anything to fd 2 of the child process. If you do not close any of the low 3 file descriptors, fd 2 of the child process is connected to whatever error stream of daemon is, so you would not see this problem, but this certainly is bad. Maybe we should check if fd 2 is sane at daemon startup, and otherwise open /dev/null for writing and dup2 it to fd 2? Currently, under --inetd mode we have freopen of stderr, but that does not help this issue. It would make die() and error() in daemon itself behave sanely but when you start the daemon with the low file descriptors closed, fileno(stderr) may be different from 2. ``` ## Matthias Lederhofer, 2006-07-12 19:28 Subject: Re: git-daemon problem Message-ID: URL: https://gitlist.dev/e/E1G0kNV-0006LK-QW%40moooo.ath.cx In-Reply-To: <7vlkqz1lja.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > This breaks the newer clients that knows how to do side-band > doesn't it? Probably, this patch is just to give a starting point where the problem could be. > Would this replacement patch help? [use write instead of safe_write] This seems to fix it. Perhaps it should be xwrite instead of write. > Maybe we should check if fd 2 is sane at daemon startup, and > otherwise open /dev/null for writing and dup2 it to fd 2? daemon startup is probably not the right place because as long as the terminal is open this will be fine. ``` ## Junio C Hamano, 2006-07-13 05:44 Subject: Re: git-daemon problem Message-ID: <7vveq2ukho.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vveq2ukho.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Matthias Lederhofer writes: > Junio C Hamano wrote: >> This breaks the newer clients that knows how to do side-band >> doesn't it? > Probably, this patch is just to give a starting point where the > problem could be. > >> Would this replacement patch help? > [use write instead of safe_write] > > This seems to fix it. Perhaps it should be xwrite instead of write. > >> Maybe we should check if fd 2 is sane at daemon startup, and >> otherwise open /dev/null for writing and dup2 it to fd 2? > daemon startup is probably not the right place because as long as the > terminal is open this will be fine. Hmph. In the part of my message you did not quote: $ git-daemon 0<&- 1>&- 2>&- --export-all /pub/git leaves listening sockets at fd 0/1 without any fd 2, and $ git-daemon 2>&- --export-all /pub/git allocates listening socket at FD 2 (because FD 0 and FD 1 are occupied). Now, after we do accept(), we spawn a subprocess in handle(), and in the child process dup2() the fd connected to the peer to fd 0 and 1 of the child process -- and we do not do anything to fd 2 of the child process. So in the latter case, my tentative patch would write error message to the listening socket -- ugh. And as you say, fd 2 might be connected to the terminal and healthy when you start the daemon, but later you can close the terminal, so there is no sane place for us to try anything sensible. The only "right" solution I could think of is to properly daemonize git-daemon when not running under --inetd mode. Close and open /dev/null the low three fds, and dissociate the process from the controlling terminal (did I forget anything else -- perhaps chdir("/") at the top?). And we keep the current behaviour of assuming the sane set of low three fds when a new option --debug is given to help people look at its stderr. The tentative patch to upload-pack would become moot at that point. Hmm? ``` ## Matthias Lederhofer, 2006-07-13 07:42 Subject: Re: git-daemon problem Message-ID: URL: https://gitlist.dev/e/E1G0vqW-00077L-1t%40moooo.ath.cx In-Reply-To: <7vveq2ukho.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Matthias Lederhofer writes: > > Junio C Hamano wrote: > >> Maybe we should check if fd 2 is sane at daemon startup, and > >> otherwise open /dev/null for writing and dup2 it to fd 2? > > daemon startup is probably not the right place because as long as the > > terminal is open this will be fine. > > Hmph. In the part of my message you did not quote: > > $ git-daemon 0<&- 1>&- 2>&- --export-all /pub/git > > leaves listening sockets at fd 0/1 without any fd 2, and > > $ git-daemon 2>&- --export-all /pub/git > > allocates listening socket at FD 2 (because FD 0 and FD 1 are > occupied). > > Now, after we do accept(), we spawn a subprocess in handle(), > and in the child process dup2() the fd connected to the peer to > fd 0 and 1 of the child process -- and we do not do anything to > fd 2 of the child process. So in the latter case, my tentative > patch would write error message to the listening socket -- ugh. > > And as you say, fd 2 might be connected to the terminal and > healthy when you start the daemon, but later you can close the > terminal, so there is no sane place for us to try anything > sensible. > > The only "right" solution I could think of is to properly > daemonize git-daemon when not running under --inetd mode. Close > and open /dev/null the low three fds, and dissociate the process > from the controlling terminal (did I forget anything else -- > perhaps chdir("/") at the top?). And we keep the current > behaviour of assuming the sane set of low three fds when a new > option --debug is given to help people look at its stderr. The > tentative patch to upload-pack would become moot at that point. > > Hmm? Now I see what you mean. Checking the fds at daemon startup should work too (using xwrite/write instead of safe_write). Is daemon(3) portable? Otherwise it should be: fork(); setsid(); chdir("/"); open("/dev/null", O_RDWR, 0); dup2(..); (looking at the daemon(3) implementations of glibc and freebsd libc) The freebsd implementation of daemon(3) ignores SIGHUP for fork() and setsid() with the comment "A SIGHUP may be thrown when the parent exits below." With the option to run in background (or the other way around -- not to run in background) should also come an option to write a pid file. ``` ## Matthias Lederhofer, 2006-07-13 10:02 Subject: [PATCH 1/5] daemon: use a custom die routine with syslog Message-ID: URL: https://gitlist.dev/e/E1G0znB-0002IH-5K%40moooo.ath.cx In-Reply-To: ``` Removed the git-daemon prefix from die() because no other call to die does this. Signed-off-by: Matthias Lederhofer --- daemon.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-) diff --git a/daemon.c b/daemon.c index e096bd7..a7636bc 100644 --- a/daemon.c +++ b/daemon.c @@ -95,6 +95,12 @@ static void loginfo(const char *err, ... va_end(params); } +static void NORETURN daemon_die(const char *err, va_list params) +{ + logreport(LOG_ERR, err, params); + exit(1); +} + static int avoid_alias(char *p) { int sl, ndot; @@ -746,17 +752,14 @@ int main(int argc, char **argv) usage(daemon_usage); } - if (log_syslog) + if (log_syslog) { openlog("git-daemon", 0, LOG_DAEMON); - - if (strict_paths && (!ok_paths || !*ok_paths)) { - if (!inetd_mode) - die("git-daemon: option --strict-paths requires a whitelist"); - - logerror("option --strict-paths requires a whitelist"); - exit (1); + set_die_routine(daemon_die); } + if (strict_paths && (!ok_paths || !*ok_paths)) + die("option --strict-paths requires a whitelist"); + if (inetd_mode) { struct sockaddr_storage ss; struct sockaddr *peer = (struct sockaddr *)&ss; -- 1.4.1.gb16f ``` ## Matthias Lederhofer, 2006-07-13 10:10 Subject: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null Message-ID: URL: https://gitlist.dev/e/E1G0znB-0002IO-61%40moooo.ath.cx In-Reply-To: ``` Signed-off-by: Matthias Lederhofer --- daemon.c | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-) diff --git a/daemon.c b/daemon.c index a7636bc..e6b1730 100644 --- a/daemon.c +++ b/daemon.c @@ -662,6 +662,24 @@ static int service_loop(int socknum, int } } +/* if any standard file descriptor is missing open it to /dev/null */ +static void sanitize_stdfds(void) +{ + int devnull = -1, i; + struct stat buf; + for (i = 0; i < 3; ++i) { + if (fstat(i, &buf) != -1) + continue; + if (devnull == -1 && + (devnull = open("/dev/null", O_RDWR, 0)) == -1) + die("open /dev/null failed: %s", strerror(errno)); + if (dup2(devnull, i) != i) + die("dup2 failed: %s", strerror(errno)); + } + if (devnull != -1) + close(devnull); +} + static int serve(int port) { int socknum, *socklist; @@ -773,5 +791,7 @@ int main(int argc, char **argv) return execute(peer); } + sanitize_stdfds(); + return serve(port); } -- 1.4.1.gb16f ``` ## Matthias Lederhofer, 2006-07-13 10:18 Subject: [PATCH 4/5] daemon: new option --pid-file= to store the pid Message-ID: URL: https://gitlist.dev/e/E1G0znB-0002IZ-7e%40moooo.ath.cx In-Reply-To: ``` Signed-off-by: Matthias Lederhofer --- daemon.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-) diff --git a/daemon.c b/daemon.c index e6b1730..4b85930 100644 --- a/daemon.c +++ b/daemon.c @@ -680,6 +680,15 @@ static void sanitize_stdfds(void) close(devnull); } +static void store_pid(const char *path) +{ + FILE *f = fopen(path, "w"); + if (!f) + die("cannot open pid file %s: %s", path, strerror(errno)); + fprintf(f, "%d\n", getpid()); + fclose(f); +} + static int serve(int port) { int socknum, *socklist; @@ -695,6 +704,7 @@ int main(int argc, char **argv) { int port = DEFAULT_GIT_PORT; int inetd_mode = 0; + const char *pid_file = NULL; int i; /* Without this we cannot rely on waitpid() to tell @@ -759,6 +769,10 @@ int main(int argc, char **argv) user_path = arg + 12; continue; } + if (!strncmp(arg, "--pid-file=", 11)) { + pid_file = arg + 11; + continue; + } if (!strcmp(arg, "--")) { ok_paths = &argv[i+1]; break; @@ -793,5 +807,8 @@ int main(int argc, char **argv) sanitize_stdfds(); + if (pid_file) + store_pid(pid_file); + return serve(port); } -- 1.4.1.gb16f ``` ## Matthias Lederhofer, 2006-07-13 10:32 Subject: [PATCH 5/5] daemon: new option --detach to run git-daemon in background Message-ID: URL: https://gitlist.dev/e/E1G0znB-0002If-8A%40moooo.ath.cx In-Reply-To: ``` Signed-off-by: Matthias Lederhofer --- daemon.c | 32 +++++++++++++++++++++++++++++++- 1 files changed, 31 insertions(+), 1 deletions(-) diff --git a/daemon.c b/daemon.c index 4b85930..9f4bc20 100644 --- a/daemon.c +++ b/daemon.c @@ -662,6 +662,27 @@ static int service_loop(int socknum, int } } +static void daemonize(void) +{ + int devnull = -1; + switch (fork()) { + case 0: + break; + case -1: + die("fork failed: %s", strerror(errno)); + default: + exit(0); + } + if (setsid() == -1) + die("setsid failed: %s", strerror(errno)); + if ((devnull = open("/dev/null", O_RDWR, 0)) == -1) + die("open /dev/null failed: %s", strerror(errno)); + if (dup2(devnull, 0) != 0 || + dup2(devnull, 1) != 1 || + dup2(devnull, 2) != 2) + die("dup2 failed: %s", strerror(errno)); +} + /* if any standard file descriptor is missing open it to /dev/null */ static void sanitize_stdfds(void) { @@ -705,6 +726,7 @@ int main(int argc, char **argv) int port = DEFAULT_GIT_PORT; int inetd_mode = 0; const char *pid_file = NULL; + int detach = 0; int i; /* Without this we cannot rely on waitpid() to tell @@ -773,6 +795,11 @@ int main(int argc, char **argv) pid_file = arg + 11; continue; } + if (!strcmp(arg, "--detach")) { + detach = 1; + log_syslog = 1; + continue; + } if (!strcmp(arg, "--")) { ok_paths = &argv[i+1]; break; @@ -805,7 +832,10 @@ int main(int argc, char **argv) return execute(peer); } - sanitize_stdfds(); + if (detach) + daemonize(); + else + sanitize_stdfds(); if (pid_file) store_pid(pid_file); -- 1.4.1.gb16f ``` ## Matthias Lederhofer, 2006-07-13 11:07 Subject: [PATCH 3/5] upload-pack: ignore write errors to stderr Message-ID: URL: https://gitlist.dev/e/E1G0znB-0002IT-77%40moooo.ath.cx In-Reply-To: ``` Signed-off-by: Matthias Lederhofer --- upload-pack.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index b18eb9b..94aa0da 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -51,6 +51,10 @@ static ssize_t send_client_data(int fd, if (fd == 3) /* emergency quit */ fd = 2; + if (fd == 2) { + xwrite(fd, data, sz); + return sz; + } return safe_write(fd, data, sz); } p = data; -- 1.4.1.gb16f ``` ## Andre Noll, 2006-07-13 11:42 Subject: Re: git-daemon problem Message-ID: <20060713114221.GU11245@skl-net.de> URL: https://gitlist.dev/e/20060713114221.GU11245%40skl-net.de In-Reply-To: <7vveq2ukho.fsf@assigned-by-dhcp.cox.net> ``` On 22:44, Junio C Hamano wrote: > The only "right" solution I could think of is to properly > daemonize git-daemon when not running under --inetd mode. Close > and open /dev/null the low three fds, and dissociate the process > from the controlling terminal (did I forget anything else -- > perhaps chdir("/") at the top?). And we keep the current > behaviour of assuming the sane set of low three fds when a new > option --debug is given to help people look at its stderr. The > tentative patch to upload-pack would become moot at that point. > > Hmm? A common solution for this problem is while (1) { int fd; fd = open("/dev/null", O_RDWR); if (fd < 0) exit(EX_OSERR); if (fd > 2) { close(fd); break; } } See http://rechner.lst.de/~okir/blackhats/node41.html Andre -- The only person who always got his work done by Friday was Robinson Crusoe ``` ## Matthias Lederhofer, 2006-07-13 11:51 Subject: Re: git-daemon problem Message-ID: URL: https://gitlist.dev/e/E1G0zj7-0001c1-8q%40moooo.ath.cx In-Reply-To: ``` Here are some patches that should solve this. Note: The first patch is not really related to this problem but I think the die error message should go to syslog when --syslog was used. (And I did not have to add more if-clauses.) Documentation will follow if the changes are ok. ``` ## Edgar Toernig, 2006-07-13 13:27 Subject: Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null Message-ID: <20060713152725.7a5081df.froese@gmx.de> URL: https://gitlist.dev/e/20060713152725.7a5081df.froese%40gmx.de In-Reply-To: ``` Matthias Lederhofer wrote: > > +/* if any standard file descriptor is missing open it to /dev/null */ > +static void sanitize_stdfds(void) > +{ > + int devnull = -1, i; > + struct stat buf; > + for (i = 0; i < 3; ++i) { > + if (fstat(i, &buf) != -1) > + continue; > + if (devnull == -1 && > + (devnull = open("/dev/null", O_RDWR, 0)) == -1) > + die("open /dev/null failed: %s", strerror(errno)); > + if (dup2(devnull, i) != i) > + die("dup2 failed: %s", strerror(errno)); > + } > + if (devnull != -1) > + close(devnull); > +} This looks broken. The open will return i as this is the lowest free fd. I don't know what POSIX says about dup2(i,i) but anyway, you close it at the end which completely defeats the intent of the function. How's this? devnull = open("/dev/null", O_RDWR, 0); if (devnull == 0) devnull = dup(devnull); if (devnull == 1) devnull = dup(devnull); if (devnull == -1) die("open/dup /dev/null failed: %s", strerror(errno)); if (devnull > 2) close(devnull); Ciao, ET. ``` ## Edgar Toernig, 2006-07-13 13:37 Subject: Re: [PATCH 5/5] daemon: new option --detach to run git-daemon in background Message-ID: <20060713153703.05f862ee.froese@gmx.de> URL: https://gitlist.dev/e/20060713153703.05f862ee.froese%40gmx.de In-Reply-To: ``` Matthias Lederhofer wrote: > > [daemonize] > + if ((devnull = open("/dev/null", O_RDWR, 0)) == -1) > + die("open /dev/null failed: %s", strerror(errno)); > + if (dup2(devnull, 0) != 0 || > + dup2(devnull, 1) != 1 || > + dup2(devnull, 2) != 2) > + die("dup2 failed: %s", strerror(errno)); > +} Hmm... leaks devnull. Why not simply close(0/1/2) and let sanitize_stdfds take care of the rest? Ciao, ET. ``` ## Matthias Lederhofer, 2006-07-13 14:04 Subject: Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null Message-ID: URL: https://gitlist.dev/e/E1G11nq-00076g-Aa%40moooo.ath.cx In-Reply-To: <20060713152725.7a5081df.froese@gmx.de> ``` Edgar Toernig wrote: > Matthias Lederhofer wrote: > > > > +/* if any standard file descriptor is missing open it to /dev/null */ > > +static void sanitize_stdfds(void) > > +{ > > + int devnull = -1, i; > > + struct stat buf; > > + for (i = 0; i < 3; ++i) { > > + if (fstat(i, &buf) != -1) > > + continue; > > + if (devnull == -1 && > > + (devnull = open("/dev/null", O_RDWR, 0)) == -1) > > + die("open /dev/null failed: %s", strerror(errno)); > > + if (dup2(devnull, i) != i) > > + die("dup2 failed: %s", strerror(errno)); > > + } > > + if (devnull != -1) > > + close(devnull); > > +} > > This looks broken. The open will return i as this is > the lowest free fd. I don't know what POSIX says > about dup2(i,i) but anyway, you close it at the end > which completely defeats the intent of the function. > > How's this? > > devnull = open("/dev/null", O_RDWR, 0); > if (devnull == 0) > devnull = dup(devnull); > if (devnull == 1) > devnull = dup(devnull); > if (devnull == -1) > die("open/dup /dev/null failed: %s", strerror(errno)); > if (devnull > 2) > close(devnull); You're right (also for the daemonize function to use sanitize_stdfds). The code looks good to me, this could also be done using a while-loop (making it a little bit shorter, I don't know what is easier to read): devnull = open("/dev/null", O_RDWR, 0); while (devnull != -1 && devnull < 2) dup(devnull); if (devnull == -1) die(".."); close(devnull); (This is similar to what Andre Noll posted.) I'll correct and resend those patches later. ``` ## Uwe Zeisberger, 2006-07-13 14:36 Subject: Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null Message-ID: <20060713143649.GA3395@informatik.uni-freiburg.de> URL: https://gitlist.dev/e/20060713143649.GA3395%40informatik.uni-freiburg.de In-Reply-To: ``` Hello Matthias, (Do you know you set the Mail-Followup-To Header? That is annoying.) > devnull = open("/dev/null", O_RDWR, 0); > while (devnull != -1 && devnull < 2) > dup(devnull); You mean devnull = dup(devnull); , don't you? > if (devnull == -1) > die(".."); > close(devnull); Best regards Uwe -- Uwe Zeisberger primes where sieve (p:xs) = [ x | x<-xs, x `rem` p /= 0 ]; \ primes = map head (iterate sieve [2..]) ``` ## Morten Welinder, 2006-07-13 15:37 Subject: Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null Message-ID: <118833cc0607130837u30c58d53lc785f56d45ef970c@mail.gmail.com> URL: https://gitlist.dev/e/118833cc0607130837u30c58d53lc785f56d45ef970c%40mail.gmail.com In-Reply-To: ``` > + if (devnull == -1 && > + (devnull = open("/dev/null", O_RDWR, 0)) == -1) > + die("open /dev/null failed: %s", strerror(errno)); > + if (dup2(devnull, i) != i) > + die("dup2 failed: %s", strerror(errno)); "die" probably won't work well at this point. Should git (and most other programs) do something like this in general? fprintf will happily write to fd=2 regardless of whether that is some critical file you opened. Morten ``` ## Matthias Lederhofer, 2006-07-13 16:03 Subject: Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null Message-ID: URL: https://gitlist.dev/e/E1G13ew-0005Iv-ST%40moooo.ath.cx In-Reply-To: <118833cc0607130837u30c58d53lc785f56d45ef970c@mail.gmail.com> ``` Morten Welinder wrote: > >+ if (devnull == -1 && > >+ (devnull = open("/dev/null", O_RDWR, 0)) == -1) > >+ die("open /dev/null failed: %s", strerror(errno)); > >+ if (dup2(devnull, i) != i) > >+ die("dup2 failed: %s", strerror(errno)); > > "die" probably won't work well at this point. At least with --syslog there will be an error message in the logs. If the user does not use --syslog and closes fd 2 it is just his own fault imho. > Should git (and most other programs) do something like this in general? > fprintf will happily write to fd=2 regardless of whether that is some critical > file you opened. I thought of that too. It might be not that important because I cannot think of anyway that this could happen accidentally or could be exploited. ``` ## Matthias Lederhofer, 2006-07-13 16:32 Subject: [PATCH 2.1/5] daemon: if one of the standard fds is missing open it to /dev/null Message-ID: URL: https://gitlist.dev/e/E1G146l-0006jI-G8%40moooo.ath.cx In-Reply-To: ``` Signed-off-by: Matthias Lederhofer --- fixed sanitize_stdfds --- daemon.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-) diff --git a/daemon.c b/daemon.c index a7636bc..01ccda3 100644 --- a/daemon.c +++ b/daemon.c @@ -662,6 +662,18 @@ static int service_loop(int socknum, int } } +/* if any standard file descriptor is missing open it to /dev/null */ +static void sanitize_stdfds(void) +{ + int fd = open("/dev/null", O_RDWR, 0); + while (fd != -1 && fd < 2) + fd = dup(fd); + if (fd == -1) + die("open /dev/null or dup failed: %s", strerror(errno)); + if (fd > 2) + close(fd); +} + static int serve(int port) { int socknum, *socklist; @@ -773,5 +785,7 @@ int main(int argc, char **argv) return execute(peer); } + sanitize_stdfds(); + return serve(port); } -- 1.4.1.g8b4b ``` ## Matthias Lederhofer, 2006-07-13 16:47 Subject: Re: [PATCH 5.1/5] daemon: new option --detach to run git-daemon in background Message-ID: URL: https://gitlist.dev/e/E1G14LJ-0007q1-Fa%40moooo.ath.cx In-Reply-To: <20060713153703.05f862ee.froese@gmx.de> ``` Signed-off-by: Matthias Lederhofer --- Edgar Toernig wrote: > Hmm... leaks devnull. Why not simply close(0/1/2) and > let sanitize_stdfds take care of the rest? --- daemon.c | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-) diff --git a/daemon.c b/daemon.c index cdc4266..e4ec676 100644 --- a/daemon.c +++ b/daemon.c @@ -674,6 +674,24 @@ static void sanitize_stdfds(void) close(fd); } +static void daemonize(void) +{ + switch (fork()) { + case 0: + break; + case -1: + die("fork failed: %s", strerror(errno)); + default: + exit(0); + } + if (setsid() == -1) + die("setsid failed: %s", strerror(errno)); + close(0); + close(1); + close(2); + sanitize_stdfds(); +} + static void store_pid(const char *path) { FILE *f = fopen(path, "w"); @@ -699,6 +717,7 @@ int main(int argc, char **argv) int port = DEFAULT_GIT_PORT; int inetd_mode = 0; const char *pid_file = NULL; + int detach = 0; int i; /* Without this we cannot rely on waitpid() to tell @@ -767,6 +786,11 @@ int main(int argc, char **argv) pid_file = arg + 11; continue; } + if (!strcmp(arg, "--detach")) { + detach = 1; + log_syslog = 1; + continue; + } if (!strcmp(arg, "--")) { ok_paths = &argv[i+1]; break; @@ -799,7 +823,10 @@ int main(int argc, char **argv) return execute(peer); } - sanitize_stdfds(); + if (detach) + daemonize(); + else + sanitize_stdfds(); if (pid_file) store_pid(pid_file); -- 1.4.1.g8b4b ``` ## Matthias Lederhofer, 2006-07-14 15:53 Subject: [PATCH] daemon: documentation for --reuseaddr, --detach and --pid-file Message-ID: URL: https://gitlist.dev/e/E1G1Pyr-0004uv-JX%40moooo.ath.cx In-Reply-To: ``` Signed-off-by: Matthias Lederhofer --- Matthias Lederhofer wrote: > Documentation will follow if the changes are ok. Here it is. I just found that --reuseaddr is not documented yet too (I could really have used that while testing the git-daemon patches...) but I have no idea how to describe it for someone who does not know what it means. Perhaps someone else has an idea. --- Documentation/git-daemon.txt | 8 +++++++- daemon.c | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt index 4c357da..f5b08a6 100644 --- a/Documentation/git-daemon.txt +++ b/Documentation/git-daemon.txt @@ -11,7 +11,7 @@ SYNOPSIS 'git-daemon' [--verbose] [--syslog] [--inetd | --port=n] [--export-all] [--timeout=n] [--init-timeout=n] [--strict-paths] [--base-path=path] [--user-path | --user-path=path] - [directory...] + [--reuseaddr] [--detach] [--pid-file=file] [directory...] DESCRIPTION ----------- @@ -82,6 +82,12 @@ OPTIONS --verbose:: Log details about the incoming connections and requested files. +--detach:: + Detach from the shell. Implies --syslog. + +--pid-file=file:: + Save the process id in 'file'. + :: A directory to add to the whitelist of allowed directories. Unless --strict-paths is specified this will also include subdirectories diff --git a/daemon.c b/daemon.c index e4ec676..810837f 100644 --- a/daemon.c +++ b/daemon.c @@ -19,7 +19,7 @@ static const char daemon_usage[] = "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" -" [--reuseaddr] [directory...]"; +" [--reuseaddr] [--detach] [--pid-file=file] [directory...]"; /* List of acceptable pathname prefixes */ static char **ok_paths = NULL; -- 1.4.1.g8b4b ```