git-daemon problem

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;

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.

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

Signed-off-by: Matthias Lederhofer <redacted>
---
 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

Signed-off-by: Matthias Lederhofer <redacted>
---
 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

Signed-off-by: Matthias Lederhofer <redacted>
---
 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

Signed-off-by: Matthias Lederhofer <redacted>
---
 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

Signed-off-by: Matthias Lederhofer <redacted>
---
 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

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.

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.

Edgar Toernig [off-list ref] wrote:

Matthias Lederhofer wrote:

quoted

+/* 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.

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

+               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

Morten Welinder [off-list ref] wrote:

quoted

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

Signed-off-by: Matthias Lederhofer <redacted>
---
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

Signed-off-by: Matthias Lederhofer <redacted>
---
Edgar Toernig [off-list ref] 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

Signed-off-by: Matthias Lederhofer <redacted>
---
Matthias Lederhofer [off-list ref] 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