From: Arran Cudbard-Bell Date: Sat, 28 Aug 2021 16:47:46 +0000 (-0500) Subject: Use a single unified exec cleanup function for cleaning up X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f7001387053d1bf16728f17a3b69ae16ab03fa4b;p=thirdparty%2Ffreeradius-server.git Use a single unified exec cleanup function for cleaning up Represent cleaned up PIDs as -1, just like we do with FDs --- diff --git a/src/lib/server/exec.c b/src/lib/server/exec.c index 31c446ddf2d..18e93479ed8 100644 --- a/src/lib/server/exec.c +++ b/src/lib/server/exec.c @@ -892,13 +892,33 @@ int fr_exec_wait_start(pid_t *pid_p, int *stdin_fd, int *stdout_fd, int *stderr_ return 0; } -/* - * Cleanup an exec after failures +/** Cleans up an exec'd process on error + * + * This function is intended to be called at any point after a successful + * #fr_exec_wait_start_io call in order to release resources and cleanup + * zombie processes. + * + * @param[in] exec state to cleanup. + * @param[in] signal If non-zero, and we think the process is still + * running, send it a signal to cause it to exit. + * The PID reaper we insert here will cleanup its + * state so it doesn't become a zombie. + * */ -static void exec_cleanup(fr_exec_state_t *exec) { +void fr_exec_cleanup(fr_exec_state_t *exec, int signal) +{ request_t *request = exec->request; - if (exec->pid) DEBUG3("Cleaning up exec state for pid %u", exec->pid); + if (exec->pid) RDEBUG3("Cleaning up exec state for pid %u", exec->pid); + + /* + * There's still an EV_PROC event installed + * for the PID remove it (there's a destructor). + */ + if (exec->ev_pid) { + talloc_const_free(exec->ev_pid); + fr_assert(!exec->ev_pid); /* Should be NULLified by destructor */ + } if (exec->stdout_fd >= 0) { if (fr_event_fd_delete(request->el, exec->stdout_fd, FR_EVENT_FILTER_IO) < 0){ @@ -916,11 +936,13 @@ static void exec_cleanup(fr_exec_state_t *exec) { exec->stderr_fd = -1; } - if (exec->pid) { - exec->pid = 0; + if (exec->pid >= 0) { + if (signal > 0) kill(exec->pid, signal); + if (fr_event_pid_reap(request->el, exec->pid) < 0) { RPERROR("Failed setting up async PID reaper, PID %u may now be a zombie", exec->pid); } + exec->pid = -1; } if (exec->ev) fr_event_timer_delete(&exec->ev); @@ -1053,10 +1075,9 @@ static void exec_timeout(UNUSED fr_event_list_t *el, UNUSED fr_time_t now, void } else { REDEBUG("Timeout running program - killing it and failing the request"); } - kill(exec->pid, SIGKILL); exec->failed = true; - exec_cleanup(exec); + fr_exec_cleanup(exec, SIGKILL); unlang_interpret_mark_runnable(exec->request); } @@ -1083,11 +1104,9 @@ static void exec_stdout_read(UNUSED fr_event_list_t *el, int fd, int flags, void if (unlikely(!remaining)) { REDEBUG("Too much output from program - killing it and failing the request"); - if (exec->pid > 0) kill(exec->pid, SIGKILL); - error: exec->failed = true; - exec_cleanup(exec); + fr_exec_cleanup(exec, SIGKILL); break; } @@ -1126,7 +1145,7 @@ static void exec_stdout_read(UNUSED fr_event_list_t *el, int fd, int flags, void close(fd); exec->stdout_fd = -1; - if (exec->pid == 0) { + if (exec->pid < 0) { /* * Child has already exited - unlang can resume */ @@ -1194,14 +1213,14 @@ int fr_exec_wait_start_io(TALLOC_CTX *ctx, fr_exec_state_t *exec, request_t *req RPEDEBUG("Failed executing program"); fail: /* - * Not done in exec_cleanup as it's + * Not done in fr_exec_cleanup as it's * usually the caller's responsibility. */ if (exec->stdin_fd >= 0) { close(exec->stdin_fd); exec->stdin_fd = -1; } - exec_cleanup(exec); + fr_exec_cleanup(exec, 0); return -1; } @@ -1209,12 +1228,12 @@ int fr_exec_wait_start_io(TALLOC_CTX *ctx, fr_exec_state_t *exec, request_t *req * Tell the event loop that it needs to wait for this PID */ if (fr_event_pid_wait(ctx, request->el, &exec->ev_pid, exec->pid, exec_waitpid, exec) < 0) { - exec->pid = 0; + exec->pid = -1; RPEDEBUG("Failed adding watcher for child process"); fail_and_close: /* - * Avoid spurious errors in exec_cleanup + * Avoid spurious errors in fr_exec_cleanup * when it tries to remove FDs from the * event loop that were never added. */ diff --git a/src/lib/server/exec.h b/src/lib/server/exec.h index 084e9887aa3..f3963a04ba0 100644 --- a/src/lib/server/exec.h +++ b/src/lib/server/exec.h @@ -90,6 +90,8 @@ int radius_exec_program(TALLOC_CTX *ctx, char *out, size_t outlen, fr_pair_list_ request_t *request, char const *cmd, fr_pair_list_t *input_pairs, bool exec_wait, bool shell_escape, fr_time_delta_t timeout) CC_HINT(nonnull (5, 6)); +void fr_exec_cleanup(fr_exec_state_t *exec, int signal); + int fr_exec_nowait(request_t *request, fr_value_box_list_t *vb_list, fr_pair_list_t *env_pairs); int fr_exec_wait_start(pid_t *pid_p, int *stdin_fd, int *stdout_fd, int *stderr_fd, diff --git a/src/lib/unlang/tmpl.c b/src/lib/unlang/tmpl.c index f909db1ea3a..861426d70e8 100644 --- a/src/lib/unlang/tmpl.c +++ b/src/lib/unlang/tmpl.c @@ -37,50 +37,6 @@ RCSID("$Id$") #include #endif -/* - * Clean up everything except the waitpid handler. - * - * If there is a waitpid handler, then this cleanup function MUST - * be called after setting the handler. - */ -static void unlang_tmpl_exec_cleanup(request_t *request) -{ - unlang_stack_t *stack = request->stack; - unlang_stack_frame_t *frame = &stack->frame[stack->depth]; - unlang_frame_state_tmpl_t *state = talloc_get_type_abort(frame->state, - unlang_frame_state_tmpl_t); - - if (state->exec.pid) RDEBUG3("Cleaning up exec state for pid %u", state->exec.pid); - - if (state->exec.stdout_fd >= 0) { - if (fr_event_fd_delete(request->el, state->exec.stdout_fd, FR_EVENT_FILTER_IO) < 0) { - RPERROR("Failed removing stdout handler"); - } - close(state->exec.stdout_fd); - state->exec.stdout_fd = -1; - } - - if (state->exec.stderr_fd >= 0) { - if (fr_event_fd_delete(request->el, state->exec.stderr_fd, FR_EVENT_FILTER_IO) < 0) { - RPERROR("Failed removing stderr handler"); - } - close(state->exec.stdout_fd); - state->exec.stdout_fd = -1; - } - - /* - * It still hasn't exited. Tell the event loop that we - * need to wait as long as necessary for the PID to exit, - * and that we don't care about the exit status. - */ - if (state->exec.pid) { - (void) fr_event_pid_wait(request->el, request->el, NULL, state->exec.pid, NULL, NULL); - state->exec.pid = 0; - } - - if (state->exec.ev) fr_event_timer_delete(&state->exec.ev); -} - /** Send a signal (usually stop) to a request * * This is typically called via an "async" action, i.e. an action @@ -97,21 +53,20 @@ static void unlang_tmpl_signal(request_t *request, unlang_stack_frame_t *frame, unlang_frame_state_tmpl_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_tmpl_t); + /* + * If we're cancelled, then kill any child processes + */ + if (action == FR_SIGNAL_CANCEL) fr_exec_cleanup(&state->exec, SIGKILL); + if (!state->signal) return; state->signal(request, state->rctx, action); /* - * If we're cancelled, then kill any child processes, and - * ignore future signals. + * If we're cancelled then disable this signal handler. + * fr_exec_cleanup should handle being called spuriously. */ - if (action == FR_SIGNAL_CANCEL) { - if (state->exec.pid > 0) kill(state->exec.pid, SIGKILL); - state->exec.failed = true; - - unlang_tmpl_exec_cleanup(request); - state->signal = NULL; - } + if (action == FR_SIGNAL_CANCEL) state->signal = NULL; } /** Push a tmpl onto the stack for evaluation @@ -208,8 +163,6 @@ static unlang_action_t unlang_tmpl_exec_wait_final(rlm_rcode_t *p_result, reques unlang_frame_state_tmpl_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_tmpl_t); - unlang_tmpl_exec_cleanup(request); - /* * The exec failed for some internal reason. We don't * care about output, and we don't care about the programs exit status. @@ -219,7 +172,7 @@ static unlang_action_t unlang_tmpl_exec_wait_final(rlm_rcode_t *p_result, reques goto resume; } - fr_assert(state->exec.pid == 0); + fr_assert(state->exec.pid < 0); if (state->exec.status != 0) { fr_assert(fr_dlist_empty(&state->box)); diff --git a/src/lib/util/event.c b/src/lib/util/event.c index 280650367de..2cb7872a6e2 100644 --- a/src/lib/util/event.c +++ b/src/lib/util/event.c @@ -2109,7 +2109,7 @@ void fr_event_service(fr_event_list_t *el) callback = pev->callback; uctx = pev->uctx; - pev->pid = 0; /* so we won't hit kevent again when it's freed */ + pev->pid = -1; /* so we won't hit kevent again when it's freed */ /* * Delete the event before calling it.