Re: [PATCH v4 15/15] daemon: opt-out on features that require posix
From: Erik Faye-Lund <hidden>
Date: 2016-06-15 22:49:50
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 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); }