From bbd3acfc678a891b9ec2b021ad9f745c34e554f7 Mon Sep 17 00:00:00 2001
From: NRK <nrk@disroot.org>
Date: Sat, 28 Jan 2023 18:38:52 +0600
Subject: rc: avoid calling free inside SIGCHLD handler

`free` is not async-signal-safe and calling it inside a signal handler
can have bad effects, as reported in the musl ML:
https://www.openwall.com/lists/musl/2023/01/23/1

the solution:

- keep track of weather remove_pid() is being called from inside a
  signal handler or not.
- if it's inside a signal handler then DO NOT call free - instead put
  that pointer into a "to be freed later" list.
- if it's not inside a signal handler then take the "to be freed later"
  list and free anything in it.

Bug: https://github.com/OpenRC/openrc/issues/589
Reported-by: Dominique MARTINET <dominique.martinet@atmark-techno.com>
---
 src/openrc/rc.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

(limited to 'src')

diff --git a/src/openrc/rc.c b/src/openrc/rc.c
index 5d2d7049..d162b514 100644
--- a/src/openrc/rc.c
+++ b/src/openrc/rc.c
@@ -87,6 +87,7 @@ static RC_HOOK hook_out;
 struct termios *termios_orig = NULL;
 
 RC_PIDLIST service_pids;
+RC_PIDLIST free_these_pids;
 
 static void
 clean_failed(void)
@@ -145,6 +146,12 @@ cleanup(void)
 		p1 = p2;
 	}
 
+	for (p1 = LIST_FIRST(&free_these_pids); p1; /* no-op */) {
+		p2 = LIST_NEXT(p1, entries);
+		free(p1);
+		p1 = p2;
+	}
+
 	rc_stringlist_free(main_hotplugged_services);
 	rc_stringlist_free(main_stop_services);
 	rc_stringlist_free(main_start_services);
@@ -350,16 +357,24 @@ add_pid(pid_t pid)
 }
 
 static void
-remove_pid(pid_t pid)
+remove_pid(pid_t pid, bool inside_signal)
 {
-	RC_PID *p;
-
-	LIST_FOREACH(p, &service_pids, entries)
-	    if (p->pid == pid) {
-		    LIST_REMOVE(p, entries);
-		    free(p);
-		    return;
-	    }
+	RC_PID *p, *tmp;
+
+	LIST_FOREACH(p, &service_pids, entries) {
+		if (p->pid == pid) {
+			LIST_REMOVE(p, entries);
+			LIST_INSERT_HEAD(&free_these_pids, p, entries);
+			break;
+		}
+	}
+	/* only call free if we're not inside a signal handler */
+	if (!inside_signal) {
+		LIST_FOREACH_SAFE(p, &free_these_pids, entries, tmp) {
+			LIST_REMOVE(p, entries);
+			free(p);
+		}
+	}
 }
 
 static void
@@ -397,7 +412,7 @@ handle_signal(int sig)
 
 		/* Remove that pid from our list */
 		if (pid > 0)
-			remove_pid(pid);
+			remove_pid(pid, true);
 		break;
 
 	case SIGWINCH:
@@ -606,7 +621,7 @@ stop:
 			add_pid(pid);
 			if (!parallel) {
 				rc_waitpid(pid);
-				remove_pid(pid);
+				remove_pid(pid, false);
 			}
 		}
 	}
@@ -672,7 +687,7 @@ do_start_services(const RC_STRINGLIST *start_services, bool parallel)
 			add_pid(pid);
 			if (!parallel) {
 				rc_waitpid(pid);
-				remove_pid(pid);
+				remove_pid(pid, false);
 			}
 		}
 	}
@@ -745,6 +760,7 @@ int main(int argc, char **argv)
 
 	applet = basename_c(argv[0]);
 	LIST_INIT(&service_pids);
+	LIST_INIT(&free_these_pids);
 	atexit(cleanup);
 	if (!applet)
 		eerrorx("arguments required");
-- 
cgit v1.2.3