Re: [PATCH v4 15/15] daemon: opt-out on features that require posix
From: Erik Faye-Lund <hidden>
Date: 2016-06-15 22:49:48
Subsystem:
kernel build + files below scripts/ (unless maintained elsewhere), the rest · Maintainers:
Nathan Chancellor, Nicolas Schier, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH v4 15/15] daemon: opt-out on features that require posix · Erik Faye-Lund <hidden>
- 2016-06-15 · Re: [PATCH v4 15/15] daemon: opt-out on features that require posix · Jonathan Nieder <hidden>
- 2016-06-15 · Re: [PATCH v4 15/15] daemon: opt-out on features that require posix · Erik Faye-Lund <hidden>
- 2016-06-15 · Re: [PATCH v4 15/15] daemon: opt-out on features that require posix · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH v4 15/15] daemon: opt-out on features that require posix · Andreas Schwab <hidden>
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); }