From 018b155ad938ed9a1867214e13b166495599d222 Mon Sep 17 00:00:00 2001 From: Denis Vlasenko Date: Tue, 6 Nov 2007 01:38:46 +0000 Subject: telnetd: fix problem with zombies (by Paul Fox ) syslogd: strip trailing NULs --- archival/unzip.c | 2 +- networking/telnetd.c | 34 +++++++++++++++++++++++----------- sysklogd/syslogd.c | 27 +++++++++++++++++---------- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/archival/unzip.c b/archival/unzip.c index 8462822..001f2e1 100644 --- a/archival/unzip.c +++ b/archival/unzip.c @@ -57,7 +57,7 @@ typedef union { uint16_t filename_len; /* 22-23 */ uint16_t extra_len; /* 24-25 */ } formatted ATTRIBUTE_PACKED; -} zip_header_t; +} zip_header_t ATTRIBUTE_PACKED; /* Check the offset of the last element, not the length. This leniency * allows for poor packing, whereby the overall struct may be too long, diff --git a/networking/telnetd.c b/networking/telnetd.c index cccf03d..108bbf4 100644 --- a/networking/telnetd.c +++ b/networking/telnetd.c @@ -279,6 +279,10 @@ make_new_session( /* make new session and process group */ setsid(); + /* Restore default signal handling */ + signal(SIGCHLD, SIG_DFL); + signal(SIGPIPE, SIG_DFL); + /* open the child's side of the tty. */ /* NB: setsid() disconnects from any previous ctty's. Therefore * we must open child's side of the tty AFTER setsid! */ @@ -302,14 +306,18 @@ make_new_session( /* Uses FILE-based I/O to stdout, but does fflush(stdout), * so should be safe with vfork. * I fear, though, that some users will have ridiculously big - * issue files, and they may block writing to fd 1. */ + * issue files, and they may block writing to fd 1, + * (parent is supposed to read it, but parent waits + * for vforked child to exec!) */ print_login_issue(issuefile, NULL); /* Exec shell / login / whatever */ login_argv[0] = loginpath; login_argv[1] = NULL; - execvp(loginpath, (char **)login_argv); - /* Safer with vfork, and we shouldn't send message + /* exec busybox applet (if PREFER_APPLETS=y), if that fails, + * exec external program */ + BB_EXECVP(loginpath, (char **)login_argv); + /* _exit is safer with vfork, and we shouldn't send message * to remote clients anyway */ _exit(1); /*bb_perror_msg_and_die("execv %s", loginpath);*/ } @@ -374,7 +382,7 @@ free_session(struct tsession *ts) #else /* !FEATURE_TELNETD_STANDALONE */ -/* Used in main() only, thus exits. */ +/* Used in main() only, thus "return 0" actually is exit(0). */ #define free_session(ts) return 0 #endif @@ -384,20 +392,22 @@ static void handle_sigchld(int sig) pid_t pid; struct tsession *ts; - pid = waitpid(-1, &sig, WNOHANG); - if (pid > 0) { + /* Looping: more than one child may have exited */ + while (1) { + pid = waitpid(-1, NULL, WNOHANG); + if (pid <= 0) + break; ts = sessions; while (ts) { if (ts->shell_pid == pid) { ts->shell_pid = -1; - return; + break; } ts = ts->next; } } } - int telnetd_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE; int telnetd_main(int argc, char **argv) { @@ -430,7 +440,7 @@ int telnetd_main(int argc, char **argv) if (!(opt & OPT_FOREGROUND)) { /* DAEMON_CHDIR_ROOT was giving inconsistent * behavior with/without -F, -i */ - bb_daemonize_or_rexec(0 /*DAEMON_CHDIR_ROOT*/, argv); + bb_daemonize_or_rexec(0 /*was DAEMON_CHDIR_ROOT*/, argv); } } /* Redirect log to syslog early, if needed */ @@ -466,6 +476,8 @@ int telnetd_main(int argc, char **argv) if (opt & OPT_WATCHCHILD) signal(SIGCHLD, handle_sigchld); + else /* prevent dead children from becoming zombies */ + signal(SIGCHLD, SIG_IGN); /* This is how the buffers are used. The arrows indicate the movement @@ -497,7 +509,7 @@ int telnetd_main(int argc, char **argv) while (ts) { struct tsession *next = ts->next; /* in case we free ts. */ if (ts->shell_pid == -1) { - /* Child died ad we detected that */ + /* Child died and we detected that */ free_session(ts); } else { if (ts->size1 > 0) /* can write to pty */ @@ -514,7 +526,7 @@ int telnetd_main(int argc, char **argv) if (!IS_INETD) { FD_SET(master_fd, &rdfdset); /* This is needed because free_session() does not - * take into account master_fd when it finds new + * take master_fd into account when it finds new * maxfd among remaining fd's */ if (master_fd > maxfd) maxfd = master_fd; diff --git a/sysklogd/syslogd.c b/sysklogd/syslogd.c index ba46792..2bf5b5c 100644 --- a/sysklogd/syslogd.c +++ b/sysklogd/syslogd.c @@ -381,8 +381,8 @@ static void parse_fac_prio_20(int pri, char *res20) } /* len parameter is used only for "is there a timestamp?" check. - * NB: some callers cheat and supply 0 when they know - * that there is no timestamp, short-cutting the test. */ + * NB: some callers cheat and supply len==0 when they know + * that there is no timestamp, short-circuiting the test. */ static void timestamp_and_log(int pri, char *msg, int len) { char *timestamp; @@ -427,10 +427,10 @@ static void split_escape_and_log(char *tmpbuf, int len) if (*p == '<') { /* Parse the magic priority number */ pri = bb_strtou(p + 1, &p, 10); - if (*p == '>') p++; - if (pri & ~(LOG_FACMASK | LOG_PRIMASK)) { + if (*p == '>') + p++; + if (pri & ~(LOG_FACMASK | LOG_PRIMASK)) pri = (LOG_USER | LOG_NOTICE); - } } while ((c = *p++)) { @@ -526,14 +526,22 @@ static void do_syslogd(void) for (;;) { size_t sz; - + read_again: sz = safe_read(sock_fd, G.recvbuf, MAX_READ - 1); - if (sz <= 0) { - //if (sz == 0) - // continue; /* EOF from unix socket??? */ + if (sz < 0) { bb_perror_msg_and_die("read from /dev/log"); } + /* Drop trailing NULs (typically there is one NUL) */ + while (1) { + if (sz == 0) + goto read_again; + if (G.recvbuf[sz-1]) + break; + sz--; + } + G.recvbuf[sz] = '\0'; /* make sure it *is* NUL terminated */ + /* TODO: maybe suppress duplicates? */ #if ENABLE_FEATURE_REMOTE_LOG /* We are not modifying log messages in any way before send */ @@ -549,7 +557,6 @@ static void do_syslogd(void) } } #endif - G.recvbuf[sz] = '\0'; split_escape_and_log(G.recvbuf, sz); } /* for */ } -- cgit v1.1