git-daemon problem

git-daemon problem

[Matthias Lederhofer]

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;

Re: git-daemon problem

[Junio C Hamano]

Matthias Lederhofer <matled@gmx.net> 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?

Re: git-daemon problem

[Junio C Hamano]

Matthias Lederhofer <matled@gmx.net> 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;

Re: git-daemon problem

[Junio C Hamano]

Matthias Lederhofer <matled@gmx.net> 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.

Re: git-daemon problem

[Matthias Lederhofer]

Junio C Hamano <junkio@cox.net> 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.

Re: git-daemon problem

[Junio C Hamano]

Matthias Lederhofer <matled@gmx.net> writes:

> Junio C Hamano <junkio@cox.net> 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?

Re: git-daemon problem

[Matthias Lederhofer]

Junio C Hamano <junkio@cox.net> wrote:
> Matthias Lederhofer <matled@gmx.net> writes:
> > Junio C Hamano <junkio@cox.net> 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.

[PATCH 1/5] daemon: use a custom die routine with syslog

[Matthias Lederhofer]

Removed the git-daemon prefix from die() because no other call to die
does this.

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
 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

[PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null

[Matthias Lederhofer]

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
 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

[PATCH 4/5] daemon: new option --pid-file=<path> to store the pid

[Matthias Lederhofer]

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
 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

[PATCH 5/5] daemon: new option --detach to run git-daemon in background

[Matthias Lederhofer]

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
 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

[PATCH 3/5] upload-pack: ignore write errors to stderr

[Matthias Lederhofer]

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
 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

Re: git-daemon problem

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: git-daemon problem

[Matthias Lederhofer]

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.

Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null

[Edgar Toernig]

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.

Re: [PATCH 5/5] daemon: new option --detach to run git-daemon in background

[Edgar Toernig]

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.

Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null

[Matthias Lederhofer]

Edgar Toernig <froese@gmx.de> 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.

Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null

[Uwe Zeisberger]

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..])

Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null

[Morten Welinder]

> +               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

Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null

[Matthias Lederhofer]

Morten Welinder <mwelinder@gmail.com> 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.

[PATCH 2.1/5] daemon: if one of the standard fds is missing open it to /dev/null

[Matthias Lederhofer]

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
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

Re: [PATCH 5.1/5] daemon: new option --detach to run git-daemon in background

[Matthias Lederhofer]

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
Edgar Toernig <froese@gmx.de> 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

[PATCH] daemon: documentation for --reuseaddr, --detach and --pid-file

[Matthias Lederhofer]

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
Matthias Lederhofer <matled@gmx.net> 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'.
+
 <directory>::
 	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