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>
This commit is contained in:
		| @@ -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; | ||||
| 	RC_PID *p, *tmp; | ||||
|  | ||||
| 	LIST_FOREACH(p, &service_pids, entries) | ||||
| 	    if (p->pid == pid) { | ||||
| 		    LIST_REMOVE(p, entries); | ||||
| 		    free(p); | ||||
| 		    return; | ||||
| 	    } | ||||
| 	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"); | ||||
|   | ||||
		Reference in New Issue
	
	Block a user