Re: [Nbd] Patches to fix a coredump and improve error reporting

[Corey Minyard <minyard@...146...>]

Ok, here's the patches again, fixed.  I also realized it's a bad idea to 
continue of setuid or setgid failed (security issue), so I changed that, 
too.

-corey

The functions getpwnam and getgrnam return NULL on error, but this
wasn't being checked.  Add checks.

Index: nbd/nbd-server.c
===================================================================
--- nbd.orig/nbd-server.c
+++ nbd/nbd-server.c
@@ -1575,11 +1575,19 @@ void dousers(void) {
 	struct group *gr;
 	if(runuser) {
 		pw=getpwnam(runuser);
+		if(!pw) {
+			msg3(LOG_DEBUG, "Invalid user name: %s", runuser);
+			exit(EXIT_FAILURE);
+		}
 		if(setuid(pw->pw_uid)<0)
 			msg3(LOG_DEBUG, "Could not set UID: %s", strerror(errno));
 	}
 	if(rungroup) {
 		gr=getgrnam(rungroup);
+		if(!gr) {
+			msg3(LOG_DEBUG, "Invalid group name: %s", rungroup);
+			exit(EXIT_FAILURE);
+		}
 		if(setgid(gr->gr_gid)<0)
 			msg3(LOG_DEBUG, "Could not set GID: %s", strerror(errno));
 	}

Delay going to daemon to as late as possible.  This way we can inform
the caller that the program didn't start, instead of just logging the
message to syslog and looking like we started ok.  It is standard
practice to wait until the program is completely configured before
daemonizing, it makes tracing down problems a lot easier.

Also, since the error reporting needed to be redone because of this,
improve the error reporting. from the setup_serve() function to print
the particular thing that failed, to make it easier to trace down
problems.

One more thing, it's probably a bad idea to continue if setuid or
setgid fails, as that's a possible security issue.  The program really
need to print a log and exit.

Index: nbd/nbd-server.c
===================================================================
--- nbd.orig/nbd-server.c
+++ nbd/nbd-server.c
@@ -1368,108 +1368,6 @@ void destroy_pid_t(gpointer data) {
 }
 
 /**
- * Go daemon (unless we specified at compile time that we didn't want this)
- * @param serve the first server of our configuration. If its port is zero,
- * 	then do not daemonize, because we're doing inetd then. This parameter
- * 	is only used to create a PID file of the form
- * 	/var/run/nbd-server.&lt;port&gt;.pid; it's not modified in any way.
- **/
-#if !defined(NODAEMON) && !defined(NOFORK)
-void daemonize(SERVER* serve) {
-	FILE*pidf;
-
-	if(daemon(0,0)<0) {
-		err("daemon");
-	}
-	if(!*pidftemplate) {
-		if(serve) {
-			strncpy(pidftemplate, "/var/run/server.%d.pid", 255);
-		} else {
-			strncpy(pidftemplate, "/var/run/server.pid", 255);
-		}
-	}
-	snprintf(pidfname, 255, pidftemplate, serve ? serve->port : 0);
-	pidf=fopen(pidfname, "w");
-	if(pidf) {
-		fprintf(pidf,"%d\n", (int)getpid());
-		fclose(pidf);
-	} else {
-		perror("fopen");
-		fprintf(stderr, "Not fatal; continuing");
-	}
-}
-#else
-#define daemonize(serve)
-#endif /* !defined(NODAEMON) && !defined(NOFORK) */
-
-/**
- * Connect a server's socket.
- *
- * @param serve the server we want to connect.
- **/
-void setup_serve(SERVER *serve) {
-	struct sockaddr_in addrin;
-	struct sigaction sa;
-	int addrinlen = sizeof(addrin);
-	int sock_flags;
-#ifndef sun
-	int yes=1;
-#else
-	char yes='1';
-#endif /* sun */
-	if ((serve->socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
-		err("socket: %m");
-
-	/* lose the pesky "Address already in use" error message */
-	if (setsockopt(serve->socket,SOL_SOCKET,SO_REUSEADDR,&yes,sizeof(int)) == -1) {
-	        err("setsockopt SO_REUSEADDR");
-	}
-	if (setsockopt(serve->socket,SOL_SOCKET,SO_KEEPALIVE,&yes,sizeof(int)) == -1) {
-		err("setsockopt SO_KEEPALIVE");
-	}
-
-	/* make the listening socket non-blocking */
-	if ((sock_flags = fcntl(serve->socket, F_GETFL, 0)) == -1) {
-		err("fcntl F_GETFL");
-	}
-	if (fcntl(serve->socket, F_SETFL, sock_flags | O_NONBLOCK) == -1) {
-		err("fcntl F_SETFL O_NONBLOCK on server socket");
-	}
-
-	DEBUG("Waiting for connections... bind, ");
-	addrin.sin_family = AF_INET;
-	addrin.sin_port = htons(serve->port);
-	addrin.sin_addr.s_addr = 0;
-	if (bind(serve->socket, (struct sockaddr *) &addrin, addrinlen) < 0)
-		err("bind: %m");
-	DEBUG("listen, ");
-	if (listen(serve->socket, 1) < 0)
-		err("listen: %m");
-	sa.sa_handler = sigchld_handler;
-	sigemptyset(&sa.sa_mask);
-	sa.sa_flags = SA_RESTART;
-	if(sigaction(SIGCHLD, &sa, NULL) == -1)
-		err("sigaction: %m");
-	sa.sa_handler = sigterm_handler;
-	sigemptyset(&sa.sa_mask);
-	sa.sa_flags = SA_RESTART;
-	if(sigaction(SIGTERM, &sa, NULL) == -1)
-		err("sigaction: %m");
-}
-
-/**
- * Connect our servers.
- **/
-void setup_servers(GArray* servers) {
-	int i;
-
-	for(i=0;i<servers->len;i++) {
-		setup_serve(&(g_array_index(servers, SERVER, i)));
-	}
-	children=g_hash_table_new_full(g_int_hash, g_int_equal, NULL, destroy_pid_t);
-}
-
-/**
  * Loop through the available servers, and serve them.
  **/
 int serveloop(GArray* servers) {
@@ -1568,6 +1466,132 @@ int serveloop(GArray* servers) {
 }
 
 /**
+ * Go daemon (unless we specified at compile time that we didn't want this)
+ * @param serve the first server of our configuration. If its port is zero,
+ * 	then do not daemonize, because we're doing inetd then. This parameter
+ * 	is only used to create a PID file of the form
+ * 	/var/run/nbd-server.&lt;port&gt;.pid; it's not modified in any way.
+ **/
+#if !defined(NODAEMON) && !defined(NOFORK)
+void daemonize(SERVER* serve) {
+	FILE*pidf;
+
+	if(daemon(0,0)<0) {
+		err("daemon");
+	}
+	if(!*pidftemplate) {
+		if(serve) {
+			strncpy(pidftemplate, "/var/run/server.%d.pid", 255);
+		} else {
+			strncpy(pidftemplate, "/var/run/server.pid", 255);
+		}
+	}
+	snprintf(pidfname, 255, pidftemplate, serve ? serve->port : 0);
+	pidf=fopen(pidfname, "w");
+	if(pidf) {
+		fprintf(pidf,"%d\n", (int)getpid());
+		fclose(pidf);
+	} else {
+		perror("fopen");
+		fprintf(stderr, "Not fatal; continuing");
+	}
+}
+#else
+#define daemonize(serve)
+#endif /* !defined(NODAEMON) && !defined(NOFORK) */
+
+
+/*
+ * All the functions below this point are run in non-daemon mode.  All
+ * the functions above run once the program has called daemonize.
+ */
+
+/**
+ * Print the server that failed, an error message, and exit.
+ *
+ * @param serve the server that failed.
+ * @param format the format string
+ **/
+void err_exit(SERVER *serve, char *format, ...)
+{
+	va_list args;
+	va_start (args, format);
+
+	g_message("Export of %s on port %d failed",
+		  serve->exportname, serve->port);
+	g_logv(G_LOG_DOMAIN, G_LOG_LEVEL_MESSAGE, format, args);
+	va_end (args);
+	exit(EXIT_FAILURE);
+}
+
+/**
+ * Connect a server's socket.
+ *
+ * @param serve the server we want to connect.
+ **/
+void setup_serve(SERVER *serve) {
+	struct sockaddr_in addrin;
+	struct sigaction sa;
+	int addrinlen = sizeof(addrin);
+	int sock_flags;
+#ifndef sun
+	int yes=1;
+#else
+	char yes='1';
+#endif /* sun */
+	if ((serve->socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
+		err_exit(serve, "socket: %m");
+
+	/* lose the pesky "Address already in use" error message */
+	if (setsockopt(serve->socket,SOL_SOCKET,SO_REUSEADDR,&yes,sizeof(int)) == -1) {
+	        err_exit(serve, "setsockopt SO_REUSEADDR");
+	}
+	if (setsockopt(serve->socket,SOL_SOCKET,SO_KEEPALIVE,&yes,sizeof(int)) == -1) {
+		err_exit(serve, "setsockopt SO_KEEPALIVE");
+	}
+
+	/* make the listening socket non-blocking */
+	if ((sock_flags = fcntl(serve->socket, F_GETFL, 0)) == -1) {
+		err_exit(serve, "fcntl F_GETFL");
+	}
+	if (fcntl(serve->socket, F_SETFL, sock_flags | O_NONBLOCK) == -1) {
+		err_exit(serve, "fcntl F_SETFL O_NONBLOCK");
+	}
+
+	DEBUG("Waiting for connections... bind, ");
+	addrin.sin_family = AF_INET;
+	addrin.sin_port = htons(serve->port);
+	addrin.sin_addr.s_addr = 0;
+	if (bind(serve->socket, (struct sockaddr *) &addrin, addrinlen) < 0)
+		err_exit(serve, "bind: %m");
+	DEBUG("listen, ");
+	if (listen(serve->socket, 1) < 0)
+		err_exit(serve, "listen: %m");
+	sa.sa_handler = sigchld_handler;
+	sigemptyset(&sa.sa_mask);
+	sa.sa_flags = SA_RESTART;
+	if(sigaction(SIGCHLD, &sa, NULL) == -1)
+		err_exit(serve, "sigaction: %m");
+	sa.sa_handler = sigterm_handler;
+	sigemptyset(&sa.sa_mask);
+	sa.sa_flags = SA_RESTART;
+	if(sigaction(SIGTERM, &sa, NULL) == -1)
+		err_exit(serve, "sigaction: %m");
+}
+
+/**
+ * Connect our servers.
+ **/
+void setup_servers(GArray* servers) {
+	int i;
+
+	for(i=0;i<servers->len;i++) {
+		setup_serve(&(g_array_index(servers, SERVER, i)));
+	}
+	children=g_hash_table_new_full(g_int_hash, g_int_equal, NULL, destroy_pid_t);
+}
+
+/**
  * Set up user-ID and/or group-ID
  **/
 void dousers(void) {
@@ -1576,20 +1600,24 @@ void dousers(void) {
 	if(runuser) {
 		pw=getpwnam(runuser);
 		if(!pw) {
-			msg3(LOG_DEBUG, "Invalid user name: %s", runuser);
+			g_message("Invalid user name: %s", runuser);
+			exit(EXIT_FAILURE);
+		}
+		if(setuid(pw->pw_uid)<0) {
+			g_message("Could not set UID: %s", strerror(errno));
 			exit(EXIT_FAILURE);
 		}
-		if(setuid(pw->pw_uid)<0)
-			msg3(LOG_DEBUG, "Could not set UID: %s", strerror(errno));
 	}
 	if(rungroup) {
 		gr=getgrnam(rungroup);
 		if(!gr) {
-			msg3(LOG_DEBUG, "Invalid group name: %s", rungroup);
+			g_message("Invalid group name: %s", rungroup);
+			exit(EXIT_FAILURE);
+		}
+		if(setgid(gr->gr_gid)<0) {
+			g_message("Could not set GID: %s", strerror(errno));
 			exit(EXIT_FAILURE);
 		}
-		if(setgid(gr->gr_gid)<0)
-			msg3(LOG_DEBUG, "Could not set GID: %s", strerror(errno));
 	}
 }
 
@@ -1646,9 +1674,9 @@ int main(int argc, char *argv[]) {
 		g_message("Nothing to do! Bye!");
 		exit(EXIT_FAILURE);
 	}
-	daemonize(serve);
 	setup_servers(servers);
 	dousers();
+	daemonize(serve);
 	serveloop(servers);
 	return 0 ;
 }