]> git.ipfire.org Git - thirdparty/freeradius-server.git/commitdiff
Use a single unified exec cleanup function for cleaning up
authorArran Cudbard-Bell <a.cudbardb@freeradius.org>
Sat, 28 Aug 2021 16:47:46 +0000 (11:47 -0500)
committerArran Cudbard-Bell <a.cudbardb@freeradius.org>
Sat, 28 Aug 2021 16:51:43 +0000 (11:51 -0500)
Represent cleaned up PIDs as -1, just like we do with FDs

src/lib/server/exec.c
src/lib/server/exec.h
src/lib/unlang/tmpl.c
src/lib/util/event.c

index 31c446ddf2d259900475a3d76b55eb6c8e6fca21..18e93479ed892f7b07c33eb551132160847c32d0 100644 (file)
@@ -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.
                 */
index 084e9887aa30c7dcaba95f23962e39b14ea38cc1..f3963a04ba092a7c2698c96650bae2a15da088a5 100644 (file)
@@ -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,
index f909db1ea3a83fbd79937144381b44b4cab0894a..861426d70e8f2e0f12a51a4e8aff71446a45c354 100644 (file)
@@ -37,50 +37,6 @@ RCSID("$Id$")
 #include <sys/wait.h>
 #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));
index 280650367de49f1d34ce71df8a4de5c2842ab05d..2cb7872a6e238dccbbc1381697917ab199ca4a0f 100644 (file)
@@ -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.