[Asterisk-code-review] app.c: make sure that no non-async-signal-safe syscalls are used afte... (asterisk[17])
Friendly Automation
asteriskteam at digium.com
Fri May 8 09:34:10 CDT 2020
Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/14392 )
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.
ASTERISK-28776
Change-Id: Idc76365c0592ee3f3b3bd72a4f48f7a098978e8e
---
M main/app.c
M main/asterisk.c
M main/strcompat.c
3 files changed, 75 insertions(+), 58 deletions(-)
Approvals:
George Joseph: Looks good to me, approved
Friendly Automation: Approved for Submit
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 6ae9219..000e1a2 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -388,6 +388,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;
@@ -1099,13 +1103,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) {
@@ -1804,10 +1802,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. */
@@ -3920,8 +3916,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..877c11c 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,69 @@
#ifndef HAVE_CLOSEFROM
void closefrom(int n)
{
- long x;
+ int maxfd;
+#ifndef _SC_OPEN_MAX
struct rlimit rl;
- DIR *dir;
- char path[16], *result;
- struct dirent *entry;
+#endif
+ struct pollfd fds[POLL_SIZE];
+ int fd=n, loopmax, i;
+#ifndef STRICT_COMPAT
+ long flags;
+#endif
- snprintf(path, sizeof(path), "/proc/%d/fd", (int) getpid());
- if ((dir = opendir(path))) {
- while ((entry = readdir(dir))) {
- /* Skip . and .. */
- if (entry->d_name[0] == '.') {
+#ifndef _SC_OPEN_MAX
+ if (getrlimit(RLIMIT_NOFILE, &rl) == -1) {
+ maxfd = -1;
+ } else {
+ maxfd = rl.rlim_cur;
+ }
+#else
+ maxfd = sysconf (_SC_OPEN_MAX);
+#endif
+
+ if (maxfd == -1 || maxfd > 65536) {
+ /* 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.
+ *
+ * sysconf as well as getrlimit can return -1 on error. Let's set
+ * maxfd to the mentioned reasonable value of 65,535 in this case.
+ */
+ maxfd = 65536;
+ }
+
+ 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);
+ 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/+/14392
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: Idc76365c0592ee3f3b3bd72a4f48f7a098978e8e
Gerrit-Change-Number: 14392
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: nappsoft <infos at nappsoft.ch>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200508/7d3db211/attachment.html>
More information about the asterisk-code-review
mailing list