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

Erik Faye-Lund [off-list ref] writes:

quoted

quoted

-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, 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.  This is especially
true if you are making the "drop-privileges" part a helper function, no?

On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano [off-list ref] wrote:

Erik Faye-Lund [off-list ref] writes:

quoted

quoted

quoted

-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);
 }

Hi,

A response to the general questions.

Erik Faye-Lund wrote:

On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano [off-list ref] wrote:

quoted

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.

Just to throw an idea out: you can also do something like

#ifndef NO_POSIX_GOODIES
struct credentials {
};
#else
struct credentials {
	struct passwd *pass;
	gid_t gid;
}
#endif

and pass a pointer to credentials around.

But also avoiding
compilation-warnings is a secondary motivation.

(void) gid;

works for this.

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?

In general, yes.

Long functions can make code much, much more difficult to read.  A
fake posix feature just requires some suspension of disbelief.

In this case (#ifdef-heavy main() vs opaque struct passwd), both
strike me as ugly.

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

Sometimes the best abstraction is the posix one and sometimes not.  I
don't think this would contradict with your planned patches, unless
they introduce #ifdefs all over the place.

quoted

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?

(See linux-2.6.git:Documentation/SubmittingPatches, section "#ifdefs
are ugly".)

The ideal: never an #ifdef within a function.  (Well, the ideal is
no #ifdef-s in .c files, but that's harder to take seriously.)

#ifndef HAVE_POSIX_GOODIES
static int drop_privileges(...)
{
	return error("--user and --group not supported on this platform");
}
#endif
static int drop_privileges(...)
{
	...
	do
	something
	...
}
#endif

would make serve() look like

static int serve(...)
{
	int socknum, *socklist;

	... setup socket ...

	if (want to drop privileges) {
		if (drop_privileges(...))
			return -1;
	}

	return service_loop(socknum, socklist);
}

which should be quite readable even to a person only interested in the
!HAVE_POSIX_GOODIES case imho.  With some code rearrangement it could
be made nicer.  Now compare:

static int serve(...)
{
	int socknum, *socklist;

	... setup socket ...

#ifdef HAVE_POSIX_GOODIES
	...
	do
	things
	...
#endif

	return service_loop(socknum, socklist);
}

Just my two cents.  Sorry I do not have something more substantive to
say.

On Mon, Oct 18, 2010 at 6:31 PM, Jonathan Nieder [off-list ref] wrote:

A response to the general questions.

Thanks!

Erik Faye-Lund wrote:

quoted

On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano [off-list ref] wrote:

quoted

quoted

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.

Just to throw an idea out: you can also do something like

#ifndef NO_POSIX_GOODIES
struct credentials {
};
#else
struct credentials {
       struct passwd *pass;
       gid_t gid;
}
#endif

and pass a pointer to credentials around.

Yes, but that structure still needs to be filled somehow. I'm not sure
how this solves anything, really. Isn't it essentially another way of
wrapping an ifdef around the parameters inside main() (at least when
I've inlined serve() into main())?

quoted

quoted

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?

(See linux-2.6.git:Documentation/SubmittingPatches, section "#ifdefs
are ugly".)

The ideal: never an #ifdef within a function.  (Well, the ideal is
no #ifdef-s in .c files, but that's harder to take seriously.)

#ifndef HAVE_POSIX_GOODIES
static int drop_privileges(...)
{
       return error("--user and --group not supported on this platform");
}
#endif
static int drop_privileges(...)
{
       ...
       do
       something
       ...
}
#endif

would make serve() look like

static int serve(...)
{
       int socknum, *socklist;

       ... setup socket ...

       if (want to drop privileges) {
               if (drop_privileges(...))
                       return -1;
       }

       return service_loop(socknum, socklist);
}

which should be quite readable even to a person only interested in the
!HAVE_POSIX_GOODIES case imho.  With some code rearrangement it could
be made nicer.  Now compare:

static int serve(...)
{
       int socknum, *socklist;

       ... setup socket ...

#ifdef HAVE_POSIX_GOODIES
       ...
       do
       things
       ...
#endif

       return service_loop(socknum, socklist);
}

Just my two cents.  Sorry I do not have something more substantive to
say.

You're leaving out the troublesome part, namely the glue between "if
(user_name)" in main(), and the "want to drop privileges"-stuff in
serve().

I could do a "struct credentials *cred = NULL;"  in main(), and assign
that inside "if (user_name)". But that'd leave a warning about
unreachable code in drop_privileges(), no?

I'm also getting the feeling that I'm being hinted at to implement
proper credential-dropping (ie filling out the windows-versions of the
code with something that makes sense for windows), but this isn't how
these things work on Windows. Daemons run as services on Windows, and
what user to run a service under is a system-administrator setting. In
fact, you can't even impersonate another user without having it's
password.

Turning git-daemon into a service is something that can be done later.
I've looked into it, and what seems to make the most sense is to have
a separate mode on git-daemon (or even another program), that starts
git-daemon as a subprocess. This is because of the way Windows
communicates with the service, requiring a message-loop that can be
terminated.

On Thu, Oct 21, 2010 at 11:16 PM, Erik Faye-Lund [off-list ref] wrote:

On Mon, Oct 18, 2010 at 6:31 PM, Jonathan Nieder [off-list ref] wrote:

quoted

Just to throw an idea out: you can also do something like

#ifndef NO_POSIX_GOODIES
struct credentials {
};
#else
struct credentials {
       struct passwd *pass;
       gid_t gid;
}
#endif

and pass a pointer to credentials around.

Yes, but that structure still needs to be filled somehow. I'm not sure
how this solves anything, really. Isn't it essentially another way of
wrapping an ifdef around the parameters inside main() (at least when
I've inlined serve() into main())?

quoted

#ifndef HAVE_POSIX_GOODIES
static int drop_privileges(...)
{
       return error("--user and --group not supported on this platform");
}
#endif
static int drop_privileges(...)
{
       ...
       do
       something
       ...
}
#endif

would make serve() look like

static int serve(...)
{
       int socknum, *socklist;

       ... setup socket ...

       if (want to drop privileges) {
               if (drop_privileges(...))
                       return -1;
       }

       return service_loop(socknum, socklist);
}

which should be quite readable even to a person only interested in the
!HAVE_POSIX_GOODIES case imho.  With some code rearrangement it could
be made nicer.  Now compare:

static int serve(...)
{
       int socknum, *socklist;

       ... setup socket ...

#ifdef HAVE_POSIX_GOODIES
       ...
       do
       things
       ...
#endif

       return service_loop(socknum, socklist);
}

Just my two cents.  Sorry I do not have something more substantive to
say.

You're leaving out the troublesome part, namely the glue between "if
(user_name)" in main(), and the "want to drop privileges"-stuff in
serve().

I could do a "struct credentials *cred = NULL;"  in main(), and assign
that inside "if (user_name)". But that'd leave a warning about
unreachable code in drop_privileges(), no?

OK, I did another stab at this, and this is the best I could come up
with right now, what do you think?

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..20ae9b4 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,33 @@ static void sanitize_stdfds(void)
 		close(fd);
 }

+#ifdef NO_POSIX_GOODIES
+
+struct credentials;
+static void drop_privileges(struct credentials *cred)
+{
+	/* nothing */
+}
+
+static void daemonize(void)
+{
+	die("--detach not supported on this platform");
+}
+
+#else
+
+struct credentials {
+	struct passwd *pass;
+	gid_t gid;
+};
+
+static void drop_privileges(struct credentials *cred)
+{
+	if (cred && initgroups(cred->pass->pw_name, cred->gid) ||
+	    setgid (cred->gid) || setuid(cred->pass->pw_uid))
+		die("cannot drop privileges");
+}
+
 static void daemonize(void)
 {
 	switch (fork()) {

@@ -955,6 +984,7 @@ static void daemonize(void)
 	close(2);
 	sanitize_stdfds();
 }
+#endif

 static void store_pid(const char *path)
 {

@@ -965,7 +995,7 @@ 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)
+static int serve(struct string_list *listen_addr, int listen_port,
struct credentials *cred)
 {
 	struct socketlist socklist = { NULL, 0, 0 };

@@ -974,10 +1004,7 @@ static int serve(struct string_list

*listen_addr, int listen_port, struct passwd
 		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");
+	drop_privileges(cred);

 	return service_loop(&socklist);
 }

@@ -989,9 +1016,7 @@ int main(int argc, char **argv)
 	int serve_mode = 0, inetd_mode = 0;
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
-	struct passwd *pass = NULL;
-	struct group *group;
-	gid_t gid = 0;
+	struct credentials *cred = NULL;
 	int i;

 	git_extract_argv0_path(argv[0]);

@@ -1079,6 +1104,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 +1118,12 @@ int main(int argc, char **argv)
 			group_name = arg + 8;
 			continue;
 		}
+#else
+		/* avoid warnings */
+		(void)user_name;
+		(void)group_name;
+		(void)detach;
+#endif
 		if (!prefixcmp(arg, "--enable=")) {
 			enable_service(arg + 9, 1);
 			continue;

@@ -1126,32 +1158,37 @@ 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");

 	if (user_name) {
-		pass = getpwnam(user_name);
-		if (!pass)
+		struct credentials c;
+		cred = &c;
+
+		c->pass = getpwnam(user_name);
+		if (!c->pass)
 			die("user not found - %s", user_name);

 		if (!group_name)
-			gid = pass->pw_gid;
+			c->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;
+			c->gid = group->gr_gid;
 		}
 	}
+#endif

 	if (strict_paths && (!ok_paths || !*ok_paths))
 		die("option --strict-paths requires a whitelist");

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

-	return serve(&listen_addr, listen_port, pass, gid);
+	return serve(&listen_addr, listen_port, cred);
 }

On Fri, Oct 22, 2010 at 12:00 AM, Erik Faye-Lund [off-list ref] wrote:

       if (user_name) {
-               pass = getpwnam(user_name);
-               if (!pass)
+               struct credentials c;
+               cred = &c;
+
+               c->pass = getpwnam(user_name);
+               if (!c->pass)
                       die("user not found - %s", user_name);

               if (!group_name)
-                       gid = pass->pw_gid;
+                       c->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;
+                       c->gid = group->gr_gid;
               }
       }

Sorry for the noise, but this is clearly incorrect and won't compile.
I guess replacing "c->" with "c." should do the trick :)

If I got this way, I'll obviously make sure it compiles! ;)

Hi Erik,

Erik Faye-Lund wrote:

OK, I did another stab at this, and this is the best I could come up
with right now, what do you think?

Much clearer; thanks.