In d0006fa78 ("treewide: use _exit() over exit()"), we replaced use of the normal exit(3) with direct calls to _exit(2). That was because glibc exit(3) made some unexpected futex() calls, which hit our seccomp profile. We've since had some bugs due to missing the extra cleanup that exit(3) implemented, for which we've added explicit cleanup calls. Specifically, we have fflush() calls in some places to avoid leaving incomplete messages on stdout/stderr, and in other places fsync_pcap_and_log() to avoid leaving incomplete log or pcap files. It's easy to forget these when adding new error paths, so instead, implement our own passt_exit() wrapper to perform vital cleanup then call _exit(2). This also provides an obvious place to add any additional cleanups we discover we need in future. Signed-off-by: David Gibson --- conf.c | 11 ++++------- log.h | 9 +++++++-- passt.c | 5 ++--- pasta.c | 10 ++++------ tap.c | 6 ++---- util.c | 24 ++++++++++++++++++------ util.h | 1 - vhost_user.c | 4 ++-- 8 files changed, 39 insertions(+), 31 deletions(-) diff --git a/conf.c b/conf.c index fdc19e81..24b44419 100644 --- a/conf.c +++ b/conf.c @@ -826,7 +826,7 @@ static void conf_ip6_local(struct ip6_ctx *ip6) * usage() - Print usage, exit with given status code * @name: Executable name * @f: Stream to print usage info to - * @status: Status code for _exit() + * @status: Status code for exit(2) */ static void usage(const char *name, FILE *f, int status) { @@ -997,8 +997,7 @@ static void usage(const char *name, FILE *f, int status) " SPEC is as described for TCP above\n" " default: none\n"); - (void)fflush(f); - _exit(status); + passt_exit(status); pasta_opts: @@ -1052,8 +1051,7 @@ pasta_opts: " --ns-mac-addr ADDR Set MAC address on tap interface\n" " --no-splice Disable inbound socket splicing\n"); - (void)fflush(f); - _exit(status); + passt_exit(status); } /** @@ -1621,8 +1619,7 @@ void conf(struct ctx *c, int argc, char **argv) FPRINTF(stdout, c->mode == MODE_PASTA ? "pasta " : "passt "); FPRINTF(stdout, VERSION_BLOB); - (void)fflush(stdout); - _exit(EXIT_SUCCESS); + passt_exit(EXIT_SUCCESS); case 15: ret = snprintf(c->ip4.ifname_out, sizeof(c->ip4.ifname_out), "%s", optarg); diff --git a/log.h b/log.h index c8473c0d..b7b20672 100644 --- a/log.h +++ b/log.h @@ -9,6 +9,11 @@ #include #include +/* This would make more sense in util.h, but because we use it in die(), that + * would cause awkward circular reference problems. + */ +void passt_exit(int status) __attribute__((noreturn)); + #define LOGFILE_SIZE_DEFAULT (1024 * 1024UL) #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */ #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE)) @@ -32,13 +37,13 @@ void logmsg_perror(int pri, const char *format, ...) #define die(...) \ do { \ err(__VA_ARGS__); \ - _exit(EXIT_FAILURE); \ + passt_exit(EXIT_FAILURE); \ } while (0) #define die_perror(...) \ do { \ err_perror(__VA_ARGS__); \ - _exit(EXIT_FAILURE); \ + passt_exit(EXIT_FAILURE); \ } while (0) extern int log_file; diff --git a/passt.c b/passt.c index 5ed88d07..cf38822f 100644 --- a/passt.c +++ b/passt.c @@ -177,8 +177,7 @@ static void exit_handler(int signal) { (void)signal; - fsync_pcap_and_log(); - _exit(EXIT_SUCCESS); + passt_exit(EXIT_SUCCESS); } /** @@ -399,7 +398,7 @@ int main(int argc, char **argv) flow_init(); if ((!c.no_udp && udp_init(&c)) || (!c.no_tcp && tcp_init(&c))) - _exit(EXIT_FAILURE); + passt_exit(EXIT_FAILURE); proto_update_l2_buf(c.guest_mac); diff --git a/pasta.c b/pasta.c index 674b5542..5c693de1 100644 --- a/pasta.c +++ b/pasta.c @@ -70,15 +70,13 @@ void pasta_child_handler(int signal) if (pasta_child_pid && !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) { if (infop.si_pid == pasta_child_pid) { - fsync_pcap_and_log(); - if (infop.si_code == CLD_EXITED) - _exit(infop.si_status); + passt_exit(infop.si_status); /* If killed by a signal, si_status is the number. * Follow common shell convention of returning it + 128. */ - _exit(infop.si_status + 128); + passt_exit(infop.si_status + 128); /* Nothing to do, detached PID namespace going away */ } @@ -511,7 +509,7 @@ void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd) if (!strncmp(ev->name, c->netns_base, sizeof(c->netns_base))) { info("Namespace %s is gone, exiting", c->netns_base); - _exit(EXIT_SUCCESS); + passt_exit(EXIT_SUCCESS); } } } @@ -539,7 +537,7 @@ void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref) return; info("Namespace %s is gone, exiting", c->netns_base); - _exit(EXIT_SUCCESS); + passt_exit(EXIT_SUCCESS); } close(fd); diff --git a/tap.c b/tap.c index e3ea61cb..9d1344b2 100644 --- a/tap.c +++ b/tap.c @@ -1149,10 +1149,8 @@ void tap_sock_reset(struct ctx *c) { info("Client connection closed%s", c->one_off ? ", exiting" : ""); - if (c->one_off) { - fsync_pcap_and_log(); - _exit(EXIT_SUCCESS); - } + if (c->one_off) + passt_exit(EXIT_SUCCESS); /* Close the connected socket, wait for a new connection */ epoll_del(c->epollfd, c->fd_tap); diff --git a/util.c b/util.c index bfeb6191..e266c396 100644 --- a/util.c +++ b/util.c @@ -555,7 +555,7 @@ void pidfile_write(int fd, pid_t pid) if (write(fd, pid_buf, n) < 0) { perror("PID file write"); - _exit(EXIT_FAILURE); + passt_exit(EXIT_FAILURE); } close(fd); @@ -592,12 +592,12 @@ int __daemon(int pidfile_fd, int devnull_fd) if (pid == -1) { perror("fork"); - _exit(EXIT_FAILURE); + passt_exit(EXIT_FAILURE); } if (pid) { pidfile_write(pidfile_fd, pid); - _exit(EXIT_SUCCESS); + passt_exit(EXIT_SUCCESS); } if (setsid() < 0 || @@ -605,7 +605,7 @@ int __daemon(int pidfile_fd, int devnull_fd) dup2(devnull_fd, STDOUT_FILENO) < 0 || dup2(devnull_fd, STDERR_FILENO) < 0 || close(devnull_fd)) - _exit(EXIT_FAILURE); + passt_exit(EXIT_FAILURE); return 0; } @@ -1225,17 +1225,29 @@ void abort_with_msg(const char *fmt, ...) } /** - * fsync_pcap_and_log() - Flush pcap and log files as needed + * passt_exit() - Perform vital cleanup and exit + * + * We don't use exit(3) because on some C library versions it can do unexpected + * things that hit our seccomp profile (e.g. futex() calls). This is a bespoke + * wrapper around _exit(2) performing just the cleanup that we need. * * #syscalls fsync */ -void fsync_pcap_and_log(void) +void passt_exit(int status) { + /* Make sure we don't leave the pcap file truncated */ if (pcap_fd != -1 && fsync(pcap_fd)) warn_perror("Failed to flush pcap file, it might be truncated"); + /* Make sure we don't leave an incomplete log */ if (log_file != -1) (void)fsync(log_file); + + /* Make sure we dont leave any messages incomplete */ + (void)fflush(stderr); + (void)fflush(stdout); + + _exit(status); } /** diff --git a/util.h b/util.h index f7a941fd..4cbb5da3 100644 --- a/util.h +++ b/util.h @@ -242,7 +242,6 @@ int read_all_buf(int fd, void *buf, size_t len); int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip); void close_open_files(int argc, char **argv); bool snprintf_check(char *str, size_t size, const char *format, ...); -void fsync_pcap_and_log(void); long clamped_scale(long x, long y, long lo, long hi, long f); /** diff --git a/vhost_user.c b/vhost_user.c index aa7c869d..f9a5646c 100644 --- a/vhost_user.c +++ b/vhost_user.c @@ -60,7 +60,7 @@ void vu_print_capabilities(void) info("{"); info(" \"type\": \"net\""); info("}"); - _exit(EXIT_SUCCESS); + passt_exit(EXIT_SUCCESS); } /** @@ -1202,7 +1202,7 @@ void vu_control_handler(struct vu_dev *vdev, int fd, uint32_t events) !vdev->context->migrate_target) { if (vdev->context->migrate_exit) { info("Migration complete, exiting"); - _exit(EXIT_SUCCESS); + passt_exit(EXIT_SUCCESS); } info("Migration complete"); -- 2.52.0