← Commit list

018b155ad

Denis Vlasenko

2007-11-06

FAILED Duration: 20s

Subject

telnetd: fix problem with zombies (by Paul Fox ) syslogd: strip trailing NULs

Binary

No binary (build failed)

Architecture & Compiler Flags

Architecture: x86_64-linux-gnu Compiler: gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0

—

Build Log

(log yok)

Commit Diff GitHub →

archival/unzip.c +1 -1 networking/telnetd.c +23 -11 sysklogd/syslogd.c +17 -10

archival/unzip.c+1 -1

@@ -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,

networking/telnetd.c+23 -11

@@ -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;

sysklogd/syslogd.c+17 -10

@@ -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 */
	}