[Nbd] Patches to fix a coredump and improve error reporting
nbd-server will coredump if the user or group specified is invalid.
The attached patch check-getpwnam-return-value.patch fixes this problem.
While working on this, I noticed that nbd-server does a lot of setup and
verification of things after calling daemonize. This could be done before
the fork and reported as a startup error, which is generally what you want
to do because it makes debugging problem easier. The delay_daemon.patch
file fixes this.
Thanks,
-corey
The functions getpwnam and getgrnam return NULL on error, but this
wasn't being checked. Add checks.
Index: nbd-2.9.6/nbd-server.c
===================================================================
--- nbd-2.9.6.orig/nbd-server.c
+++ nbd-2.9.6/nbd-server.c
@@ -1536,11 +1536,19 @@ void dousers(void) {
struct group *gr;
if(runuser) {
pw=getpwnam(runuser);
+ if(!pw) {
+ g_message("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) {
+ g_message("Invalid group name: %s", rungroup);
+ exit(EXIT_FAILURE);
+ }
if(setgid(gr->gr_gid)<0)
msg3(LOG_DEBUG, "Could not set GID: %s", strerror(errno));
}
@@ -1596,9 +1604,9 @@ int main(int argc, char *argv[]) {
g_message("Nothing to do! Bye!");
exit(EXIT_FAILURE);
}
+ dousers();
daemonize(serve);
setup_servers(servers);
- dousers();
serveloop(servers);
return 0 ;
}
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.
Index: nbd-2.9.6/nbd-server.c
===================================================================
--- nbd-2.9.6.orig/nbd-server.c
+++ nbd-2.9.6/nbd-server.c
@@ -1335,6 +1335,95 @@ void destroy_pid_t(gpointer data) {
}
/**
+ * Loop through the available servers, and serve them.
+ **/
+int serveloop(GArray* servers) {
+ struct sockaddr_in addrin;
+ socklen_t addrinlen=sizeof(addrin);
+ SERVER *serve;
+ int i;
+ int max;
+ int sock;
+ fd_set mset;
+ fd_set rset;
+ struct timeval tv;
+
+ /*
+ * Set up the master fd_set. The set of descriptors we need
+ * to select() for never changes anyway and it buys us a *lot*
+ * of time to only build this once. However, if we ever choose
+ * to not fork() for clients anymore, we may have to revisit
+ * this.
+ */
+ max=0;
+ FD_ZERO(&mset);
+ for(i=0;i<servers->len;i++) {
+ sock=(g_array_index(servers, SERVER, i)).socket;
+ FD_SET(sock, &mset);
+ max=sock>max?sock:max;
+ }
+ for(;;) {
+ CLIENT *client;
+ int net;
+ pid_t *pid;
+
+ memcpy(&rset, &mset, sizeof(fd_set));
+ tv.tv_sec=0;
+ tv.tv_usec=500;
+ if(select(max+1, &rset, NULL, NULL, &tv)>0) {
+ DEBUG("accept, ");
+ for(i=0;i<servers->len;i++) {
+ serve=&(g_array_index(servers, SERVER, i));
+ if(FD_ISSET(serve->socket, &rset)) {
+ if ((net=accept(serve->socket, (struct sockaddr *) &addrin, &addrinlen)) < 0)
+ err("accept: %m");
+
+ client = g_malloc(sizeof(CLIENT));
+ client->server=serve;
+ client->exportsize=OFFT_MAX;
+ client->net=net;
+ set_peername(net, client);
+ if (!authorized_client(client)) {
+ msg2(LOG_INFO,"Unauthorized client") ;
+ close(net);
+ continue;
+ }
+ msg2(LOG_INFO,"Authorized client") ;
+ pid=g_malloc(sizeof(pid_t));
+#ifndef NOFORK
+ if ((*pid=fork())<0) {
+ msg3(LOG_INFO,"Could not fork (%s)",strerror(errno)) ;
+ close(net);
+ continue;
+ }
+ if (*pid>0) { /* parent */
+ close(net);
+ g_hash_table_insert(children, pid, pid);
+ continue;
+ }
+ /* child */
+ g_hash_table_destroy(children);
+ for(i=0;i<servers->len,serve=(g_array_index(servers, SERVER*, i));i++) {
+ close(serve->socket);
+ }
+ /* FALSE does not free the
+ actual data. This is required,
+ because the client has a
+ direct reference into that
+ data, and otherwise we get a
+ segfault... */
+ g_array_free(servers, FALSE);
+#endif // NOFORK
+ msg2(LOG_INFO,"Starting to serve");
+ serveconnection(client);
+ exit(0);
+ }
+ }
+ }
+ }
+}
+
+/**
* 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
@@ -1372,6 +1461,30 @@ void daemonize(SERVER* serve) {
#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.
*
@@ -1388,22 +1501,22 @@ void setup_serve(SERVER *serve) {
char yes='1';
#endif /* sun */
if ((serve->socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
- err("socket: %m");
+ 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("setsockopt SO_REUSEADDR");
+ err_exit(serve, "setsockopt SO_REUSEADDR");
}
if (setsockopt(serve->socket,SOL_SOCKET,SO_KEEPALIVE,&yes,sizeof(int)) == -1) {
- err("setsockopt SO_KEEPALIVE");
+ err_exit(serve, "setsockopt SO_KEEPALIVE");
}
/* make the listening socket non-blocking */
if ((sock_flags = fcntl(serve->socket, F_GETFL, 0)) == -1) {
- err("fcntl F_GETFL");
+ err_exit(serve, "fcntl F_GETFL");
}
if (fcntl(serve->socket, F_SETFL, sock_flags | O_NONBLOCK) == -1) {
- err("fcntl F_SETFL O_NONBLOCK");
+ err_exit(serve, "fcntl F_SETFL O_NONBLOCK");
}
DEBUG("Waiting for connections... bind, ");
@@ -1411,20 +1524,20 @@ void setup_serve(SERVER *serve) {
addrin.sin_port = htons(serve->port);
addrin.sin_addr.s_addr = 0;
if (bind(serve->socket, (struct sockaddr *) &addrin, addrinlen) < 0)
- err("bind: %m");
+ err_exit(serve, "bind: %m");
DEBUG("listen, ");
if (listen(serve->socket, 1) < 0)
- err("listen: %m");
+ 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("sigaction: %m");
+ 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("sigaction: %m");
+ err_exit(serve, "sigaction: %m");
}
/**
@@ -1440,95 +1553,6 @@ void setup_servers(GArray* servers) {
}
/**
- * Loop through the available servers, and serve them.
- **/
-int serveloop(GArray* servers) {
- struct sockaddr_in addrin;
- socklen_t addrinlen=sizeof(addrin);
- SERVER *serve;
- int i;
- int max;
- int sock;
- fd_set mset;
- fd_set rset;
- struct timeval tv;
-
- /*
- * Set up the master fd_set. The set of descriptors we need
- * to select() for never changes anyway and it buys us a *lot*
- * of time to only build this once. However, if we ever choose
- * to not fork() for clients anymore, we may have to revisit
- * this.
- */
- max=0;
- FD_ZERO(&mset);
- for(i=0;i<servers->len;i++) {
- sock=(g_array_index(servers, SERVER, i)).socket;
- FD_SET(sock, &mset);
- max=sock>max?sock:max;
- }
- for(;;) {
- CLIENT *client;
- int net;
- pid_t *pid;
-
- memcpy(&rset, &mset, sizeof(fd_set));
- tv.tv_sec=0;
- tv.tv_usec=500;
- if(select(max+1, &rset, NULL, NULL, &tv)>0) {
- DEBUG("accept, ");
- for(i=0;i<servers->len;i++) {
- serve=&(g_array_index(servers, SERVER, i));
- if(FD_ISSET(serve->socket, &rset)) {
- if ((net=accept(serve->socket, (struct sockaddr *) &addrin, &addrinlen)) < 0)
- err("accept: %m");
-
- client = g_malloc(sizeof(CLIENT));
- client->server=serve;
- client->exportsize=OFFT_MAX;
- client->net=net;
- set_peername(net, client);
- if (!authorized_client(client)) {
- msg2(LOG_INFO,"Unauthorized client") ;
- close(net);
- continue;
- }
- msg2(LOG_INFO,"Authorized client") ;
- pid=g_malloc(sizeof(pid_t));
-#ifndef NOFORK
- if ((*pid=fork())<0) {
- msg3(LOG_INFO,"Could not fork (%s)",strerror(errno)) ;
- close(net);
- continue;
- }
- if (*pid>0) { /* parent */
- close(net);
- g_hash_table_insert(children, pid, pid);
- continue;
- }
- /* child */
- g_hash_table_destroy(children);
- for(i=0;i<servers->len,serve=(g_array_index(servers, SERVER*, i));i++) {
- close(serve->socket);
- }
- /* FALSE does not free the
- actual data. This is required,
- because the client has a
- direct reference into that
- data, and otherwise we get a
- segfault... */
- g_array_free(servers, FALSE);
-#endif // NOFORK
- msg2(LOG_INFO,"Starting to serve");
- serveconnection(client);
- exit(0);
- }
- }
- }
- }
-}
-
-/**
* Set up user-ID and/or group-ID
**/
void dousers(void) {
@@ -1605,8 +1629,8 @@ int main(int argc, char *argv[]) {
exit(EXIT_FAILURE);
}
dousers();
- daemonize(serve);
setup_servers(servers);
+ daemonize(serve);
serveloop(servers);
return 0 ;
}
Reply to: