[Asterisk-code-review] app.c: make sure that no non-async-signal-safe syscalls are used afte... (asterisk[16])

Pirmin Walthert asteriskteam at digium.com
Tue Apr 14 11:06:54 CDT 2020


Pirmin Walthert has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/14209 )


Change subject: app.c: make sure that no non-async-signal-safe syscalls are used after fork before exec
......................................................................

app.c: make sure that no non-async-signal-safe syscalls are used after
fork before exec

Posix does only allow async-signal-safe syscalls after fork before exec.
As asterisk ignores this, functions like TrySystem or System sometimes
end up in a deadlocked child process. The patch prevents the use of
non-async-signal-safe syscalls.

Change-Id: Idc76365c0592ee3f3b3bd72a4f48f7a098978e8e
---
M main/app.c
M main/asterisk.c
M main/strcompat.c
3 files changed, 50 insertions(+), 60 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/09/14209/1

diff --git a/main/app.c b/main/app.c
index ae3f416..09c0123 100644
--- a/main/app.c
+++ b/main/app.c
@@ -81,6 +81,9 @@
 
 static AST_LIST_HEAD_STATIC(zombies, zombie);
 
+#ifdef HAVE_CAP
+static cap_t child_cap;
+#endif
 /*
  * @{ \brief Define \ref stasis topic objects
  */
@@ -3003,12 +3006,7 @@
 	} else {
 		/* Child */
 #ifdef HAVE_CAP
-		cap_t cap = cap_from_text("cap_net_admin-eip");
-
-		if (cap_set_proc(cap)) {
-			ast_log(LOG_WARNING, "Unable to remove capabilities.\n");
-		}
-		cap_free(cap);
+		cap_set_proc(child_cap);
 #endif
 
 		/* Before we unblock our signals, return our trapped signals back to the defaults */
@@ -3118,6 +3116,9 @@
 
 static void app_cleanup(void)
 {
+#ifdef HAS_CAP
+	cap_free(child_cap);
+#endif
 	ao2_cleanup(queue_topic_pool);
 	queue_topic_pool = NULL;
 	ao2_cleanup(queue_topic_all);
@@ -3127,7 +3128,9 @@
 int app_init(void)
 {
 	ast_register_cleanup(app_cleanup);
-
+#ifdef HAVE_CAP
+	child_cap = cap_from_text("cap_net_admin-eip");
+#endif
 	queue_topic_all = stasis_topic_create("queue:all");
 	if (!queue_topic_all) {
 		return -1;
diff --git a/main/asterisk.c b/main/asterisk.c
index 037c7cd..c0c345d 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -385,6 +385,10 @@
 
 static char randompool[256];
 
+#ifdef HAVE_CAP
+static cap_t child_cap;
+#endif
+
 static int sig_alert_pipe[2] = { -1, -1 };
 static struct {
 	 unsigned int need_reload:1;
@@ -1096,13 +1100,7 @@
 
 	if (pid == 0) {
 #ifdef HAVE_CAP
-		cap_t cap = cap_from_text("cap_net_admin-eip");
-
-		if (cap_set_proc(cap)) {
-			/* Careful with order! Logging cannot happen after we close FDs */
-			ast_log(LOG_WARNING, "Unable to remove capabilities.\n");
-		}
-		cap_free(cap);
+		cap_set_proc(child_cap);
 #endif
 #ifdef HAVE_WORKING_FORK
 		if (ast_opt_high_priority) {
@@ -1801,10 +1799,8 @@
 	if (pri) {
 		sched.sched_priority = 10;
 		if (sched_setscheduler(0, SCHED_RR, &sched)) {
-			ast_log(LOG_WARNING, "Unable to set high priority\n");
 			return -1;
-		} else
-			ast_verb(1, "Set to realtime thread\n");
+		}
 	} else {
 		sched.sched_priority = 0;
 		/* According to the manpage, these parameters can never fail. */
@@ -3917,8 +3913,14 @@
 		exit(1);
 	}
 
+#ifdef HAVE_CAP
+	child_cap = cap_from_text("cap_net_admin-eip");
+#endif
 	/* Not a remote console? Start the daemon. */
 	asterisk_daemon(isroot, runuser, rungroup);
+#ifdef HAS_CAP
+	cap_free(child_cap);
+#endif
 	return 0;
 }
 
diff --git a/main/strcompat.c b/main/strcompat.c
index 0034c21..623808c 100644
--- a/main/strcompat.c
+++ b/main/strcompat.c
@@ -38,6 +38,8 @@
 
 #include "asterisk/utils.h"
 
+#define POLL_SIZE 1024
+
 #ifndef HAVE_STRSEP
 char *strsep(char **str, const char *delims)
 {
@@ -426,59 +428,42 @@
 #ifndef HAVE_CLOSEFROM
 void closefrom(int n)
 {
-	long x;
-	struct rlimit rl;
-	DIR *dir;
-	char path[16], *result;
-	struct dirent *entry;
-
-	snprintf(path, sizeof(path), "/proc/%d/fd", (int) getpid());
-	if ((dir = opendir(path))) {
-		while ((entry = readdir(dir))) {
-			/* Skip . and .. */
-			if (entry->d_name[0] == '.') {
+	int maxfd = sysconf (_SC_OPEN_MAX);
+	if (maxfd == -1)
+		maxfd = 1024;
+	if (maxfd > 65536)
+		maxfd = 65536;
+	struct pollfd fds[POLL_SIZE];
+	int fd=n, loopmax, i;
+	while (fd<maxfd) {
+		loopmax = maxfd - fd;
+		if (loopmax>POLL_SIZE) {
+			loopmax = POLL_SIZE;
+		}
+		for (i=0;i<loopmax;i++) {
+			fds[i].fd = fd+i;
+			fds[i].events = 0;
+		}
+		poll(fds, loopmax, 0);
+		for (i=0;i<loopmax;i++) {
+			if (fds[i].revents == POLLNVAL) {
 				continue;
 			}
-			if ((x = strtol(entry->d_name, &result, 10)) && x >= n) {
 #ifdef STRICT_COMPAT
-				close(x);
+			close(fds[i].fd);
 #else
-				/* This isn't strictly compatible, but it's actually faster
-				 * for our purposes to set the CLOEXEC flag than to close
-				 * file descriptors.
-				 */
-				long flags = fcntl(x, F_GETFD);
-				if (flags == -1 && errno == EBADF) {
-					continue;
-				}
-				fcntl(x, F_SETFD, flags | FD_CLOEXEC);
-#endif
-			}
-		}
-		closedir(dir);
-	} else {
-		getrlimit(RLIMIT_NOFILE, &rl);
-		if (rl.rlim_cur > 65535) {
-			/* A more reasonable value.  Consider that the primary source of
-			 * file descriptors in Asterisk are UDP sockets, of which we are
-			 * limited to 65,535 per address.  We additionally limit that down
-			 * to about 10,000 sockets per protocol.  While the kernel will
-			 * allow us to set the fileno limit higher (up to 4.2 billion),
-			 * there really is no practical reason for it to be that high.
+			/* This isn't strictly compatible, but it's actually faster
+			 * for our purposes to set the CLOEXEC flag than to close
+			 * file descriptors.
 			 */
-			rl.rlim_cur = 65535;
-		}
-		for (x = n; x < rl.rlim_cur; x++) {
-#ifdef STRICT_COMPAT
-			close(x);
-#else
-			long flags = fcntl(x, F_GETFD);
+			long flags = fcntl(fds[i].fd, F_GETFD);
 			if (flags == -1 && errno == EBADF) {
 				continue;
 			}
-			fcntl(x, F_SETFD, flags | FD_CLOEXEC);
+			fcntl(fds[i].fd, F_SETFD, flags | FD_CLOEXEC);
 #endif
 		}
+		fd+=loopmax;
 	}
 }
 #endif

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/14209
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Idc76365c0592ee3f3b3bd72a4f48f7a098978e8e
Gerrit-Change-Number: 14209
Gerrit-PatchSet: 1
Gerrit-Owner: Pirmin Walthert <infos at nappsoft.ch>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200414/dcae981a/attachment.html>


More information about the asterisk-code-review mailing list