Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

[Erik Faye-Lund <kusmabite@gmail.com>]

On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>>> -static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>>>> +#ifndef NO_POSIX_GOODIES
>>>> +static struct passwd *pass;
>>>> +static gid_t gid;
>>>> +#endif
>>>> +
>>>> +static int serve(struct string_list *listen_addr, int listen_port)
>>>>  {
>>>>       struct socketlist socklist = { NULL, 0, 0 };
>>>>
>>>
>>> This is ugly.  Why did you need to make the arguments file-scope static?
>>>
>>
>> To avoid having different signatures for the serve-function dependent
>> on NO_POSIX_GOODIES.
>
> Why does the signature even have to be different between the two to begin
> with? I _think_ you have gid_t over there

We don't, so this is the primary reason. But also avoiding
compilation-warnings is a secondary motivation.

> although you might not have
> "struct passwd", in which case you can just define an empty one that your
> alternate implementation is not going to use anyway.

We do, so this becomes a bit of a hypothetical question. But would you
seriously consider pretending to have a posix-feature less ugly than
inlining a function that is only used once?

(I'm going a little off-topic here, I hope that's OK)
I'm not too happy with some of the
pretend-really-hard-to-be-posix-magic around in the Windows-port. In
fact, I have some patches to reduce posixness in some areas, while
getting rid of some code in mingw.c. Would such patches be welcome, or
is pretend-to-be-posix the governing portability approach? In some
cases, this comes at the expense of some performance (and quite a bit
of added cludge), which is a bit contradictory to the Git design IMO.

> This is especially
> true if you are making the "drop-privileges" part a helper function, no?

I don't follow this part. What exactly becomes more true by having a
drop-privileges function?

Anyway, I'm pretty pleased with how this turned out after inlining
serve() into main(), what do you think about this? I've also moved the
reordering of usage-string into a new patch that makes inetd_mode and
detach incompatible (they already are, it's just not checked for or
documented).

diff --git a/Makefile b/Makefile
index 46034bf..53986b1 100644
--- a/Makefile
+++ b/Makefile
@@ -401,6 +401,7 @@ EXTRA_PROGRAMS =
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS += $(EXTRA_PROGRAMS)

+PROGRAM_OBJS += daemon.o
 PROGRAM_OBJS += fast-import.o
 PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += shell.o
@@ -1066,7 +1067,6 @@ ifeq ($(uname_S),Windows)
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1077,6 +1077,7 @@ ifeq ($(uname_S),Windows)
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	NATIVE_CRLF = YesPlease

 	CC = compat/vcbuild/scripts/clink.pl
@@ -1119,7 +1120,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1130,6 +1130,9 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
 	ETAGS_TARGET = ETAGS
+	NO_INET_PTON = YesPlease
+	NO_INET_NTOP = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
-Icompat/fnmatch -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
@@ -1249,9 +1252,6 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz

-ifndef NO_POSIX_ONLY_PROGRAMS
-	PROGRAM_OBJS += daemon.o
-endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
@@ -1419,6 +1419,10 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif

+ifdef NO_POSIX_GOODIES
+	BASIC_CFLAGS += -DNO_POSIX_GOODIES
+endif
+
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
diff --git a/daemon.c b/daemon.c
index b7f3874..589bd04 100644
--- a/daemon.c
+++ b/daemon.c
@@ -26,7 +26,9 @@ static const char daemon_usage[] =
 "           [--reuseaddr] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
+#ifndef NO_POSIX_GOODIES
 "                      [--detach] [--user=user [--group=group]]\n"
+#endif
 "           [directory...]";

 /* List of acceptable pathname prefixes */
@@ -938,6 +940,14 @@ static void sanitize_stdfds(void)
 		close(fd);
 }

+#ifndef NO_POSIX_GOODIES
+static void drop_privileges(struct passwd *pass, gid_t gid)
+{
+	if (initgroups(pass->pw_name, gid) || setgid (gid) ||
+	     setuid(pass->pw_uid))
+		die("cannot drop privileges");
+}
+
 static void daemonize(void)
 {
 	switch (fork()) {
@@ -955,6 +965,7 @@ static void daemonize(void)
 	close(2);
 	sanitize_stdfds();
 }
+#endif

 static void store_pid(const char *path)
 {
@@ -965,33 +976,19 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }

-static int serve(struct string_list *listen_addr, int listen_port,
struct passwd *pass, gid_t gid)
-{
-	struct socketlist socklist = { NULL, 0, 0 };
-
-	socksetup(listen_addr, listen_port, &socklist);
-	if (socklist.nr == 0)
-		die("unable to allocate any listen sockets on port %u",
-		    listen_port);
-
-	if (pass && gid &&
-	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
-	     setuid(pass->pw_uid)))
-		die("cannot drop privileges");
-
-	return service_loop(&socklist);
-}
-
 int main(int argc, char **argv)
 {
 	int listen_port = 0;
 	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
+	struct socketlist socklist = { NULL, 0, 0 };
 	int serve_mode = 0, inetd_mode = 0;
-	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
+	const char *pid_file = NULL;
+#ifndef NO_POSIX_GOODIES
+	const char *user_name = NULL, *group_name = NULL;
 	int detach = 0;
 	struct passwd *pass = NULL;
-	struct group *group;
 	gid_t gid = 0;
+#endif
 	int i;

 	git_extract_argv0_path(argv[0]);
@@ -1079,6 +1076,7 @@ int main(int argc, char **argv)
 			pid_file = arg + 11;
 			continue;
 		}
+#ifndef NO_POSIX_GOODIES
 		if (!strcmp(arg, "--detach")) {
 			detach = 1;
 			log_syslog = 1;
@@ -1092,6 +1090,7 @@ int main(int argc, char **argv)
 			group_name = arg + 8;
 			continue;
 		}
+#endif
 		if (!prefixcmp(arg, "--enable=")) {
 			enable_service(arg + 9, 1);
 			continue;
@@ -1126,14 +1125,15 @@ int main(int argc, char **argv)
 		/* avoid splitting a message in the middle */
 		setvbuf(stderr, NULL, _IOFBF, 4096);

-	if (inetd_mode && (detach || group_name || user_name))
-		die("--detach, --user and --group are incompatible with --inetd");
-
 	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;

+#ifndef NO_POSIX_GOODIES
+	if (inetd_mode && (detach || group_name || user_name))
+		die("--detach, --user and --group are incompatible with --inetd");
+
 	if (group_name && !user_name)
 		die("--group supplied without --user");

@@ -1145,13 +1145,14 @@ int main(int argc, char **argv)
 		if (!group_name)
 			gid = pass->pw_gid;
 		else {
-			group = getgrnam(group_name);
+			struct group *group = getgrnam(group_name);
 			if (!group)
 				die("group not found - %s", group_name);

 			gid = group->gr_gid;
 		}
 	}
+#endif

 	if (strict_paths && (!ok_paths || !*ok_paths))
 		die("option --strict-paths requires a whitelist");
@@ -1168,11 +1169,13 @@ int main(int argc, char **argv)
 	if (inetd_mode || serve_mode)
 		return execute();

+#ifndef NO_POSIX_GOODIES
 	if (detach) {
 		daemonize();
 		loginfo("Ready to rumble");
 	}
 	else
+#endif
 		sanitize_stdfds();

 	if (pid_file)
@@ -1185,5 +1188,15 @@ int main(int argc, char **argv)
 	cld_argv[argc] = "--serve";
 	cld_argv[argc+1] = NULL;

-	return serve(&listen_addr, listen_port, pass, gid);
+	socksetup(&listen_addr, listen_port, &socklist);
+	if (socklist.nr == 0)
+		die("unable to allocate any listen sockets on port %u",
+		    listen_port);
+
+#ifndef NO_POSIX_GOODIES
+	if (pass && gid)
+		drop_privileges(pass, gid);
+#endif
+
+	return service_loop(&socklist);
 }