Ensure stack frame state is freed in a deterministic way

diff --git a/src/lib/io/worker.c b/src/lib/io/worker.c
index 5ddfde7cdc60a9b22a81403cc6b6ee7ecfcf89e6..a0459c33a4c4ea0deab214af70bea1ba51837e2b 100644 (file)
--- a/src/lib/io/worker.c
+++ b/src/lib/io/worker.c
@@ -971,9 +971,10 @@ static void _worker_request_done_external(request_t *request, UNUSED rlm_rcode_t
         */
        fr_assert_msg(request_is_internal(request) || request_is_detached(request) || (request->log.unlang_indent == 0),
                      "Request %s bad log indentation - expected 0 got %u", request->name, request->log.unlang_indent);
-       fr_assert_msg((request->master_state == REQUEST_STOP_PROCESSING) || !unlang_interpret_is_resumable(request),
+       fr_assert_msg(!unlang_interpret_is_resumable(request),
                      "Request %s is marked as yielded at end of processing", request->name);
-
+       fr_assert_msg(unlang_interpret_stack_depth(request) == 0,
+                     "Request %s stack depth %u > 0", request->name, unlang_interpret_stack_depth(request));
        RDEBUG("Done request");
 
        worker_send_reply(worker, request, request->master_state == REQUEST_STOP_PROCESSING ? 1 : 0, fr_time());
@@ -1069,6 +1070,12 @@ static void _worker_request_stop(request_t *request, void *uctx)
                fr_time_tracking_resume(&request->async->tracking, fr_time());
        }
 
+       /*
+        *      Let everyone know the request is being
+        *      stopped.
+        */
+       request->master_state = REQUEST_STOP_PROCESSING;
+
        /*
         *      If the request is in the runnable queue
         *      yank it back out, so it's not "runnable"

diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c
index 8af40cc90c39ff39cee3f7458711f606d4783765..d1ed246e23d9e15ae428b3ac708d8cd964ce7b70 100644 (file)
--- a/src/lib/unlang/interpret.c
+++ b/src/lib/unlang/interpret.c
@@ -791,58 +791,6 @@ void *unlang_interpret_stack_alloc(TALLOC_CTX *ctx)
        return stack;
 }
 
-/** Send a signal (usually stop) to a request
- *
- * This is typically called via an "async" action, i.e. an action
- * outside of the normal processing of the request.
- *
- * If there is no #unlang_module_signal_t callback defined, the action is ignored.
- *
- * The signaling stops at the "limit" frame.  This is so that keywords
- * such as "timeout" and "limit" can signal frames *lower* than theirs
- * to stop, but then continue with their own work.
- *
- * @param[in] request          The current request.
- * @param[in] action           to signal.
- * @param[in] limit            the frame at which to stop signaling.
- */
-static inline CC_HINT(always_inline) void frame_signal(request_t *request, fr_state_signal_t action, int limit)
-{
-       unlang_stack_frame_t    *frame;
-       unlang_stack_t          *stack = request->stack;
-       int                     i, depth = stack->depth;
-
-       (void)talloc_get_type_abort(request, request_t);        /* Check the request hasn't already been freed */
-
-       fr_assert(stack->depth > 0);
-
-       /*
-        *      Walk back up the stack, calling signal handlers
-        *      to cancel any pending operations and free/release
-        *      any resources.
-        *
-        *      There may be multiple resumption points in the
-        *      stack, as modules can push xlats and function
-        *      calls.
-        */
-       for (i = depth; i > limit; i--) {
-               stack->depth = i;                       /* We could also pass in the frame to the signal function */
-               frame = &stack->frame[stack->depth];
-
-               if (is_top_frame(frame)) continue;              /* Skip top frames as they have no instruction */
-
-               /*
-                *      Be gracious in errors.
-                */
-               if (!is_yielded(frame)) continue;
-
-               if (!frame->signal) continue;
-
-               frame->signal(request, frame, action);
-       }
-       stack->depth = depth;                           /* Reset */
-}
-
 /** Indicate to the caller of the interpreter that this request is complete
  *
  */
@@ -901,22 +849,72 @@ void unlang_interpret_request_detach(request_t *request)
  *
  * If there is no #unlang_module_signal_t callback defined, the action is ignored.
  *
+ * The signaling stops at the "limit" frame.  This is so that keywords
+ * such as "timeout" and "limit" can signal frames *lower* than theirs
+ * to stop, but then continue with their own work.
+ *
  * @param[in] request          The current request.
  * @param[in] action           to signal.
+ * @param[in] limit            the frame at which to stop signaling.
  */
-void unlang_interpret_signal(request_t *request, fr_state_signal_t action)
+static inline CC_HINT(always_inline) void frame_signal(request_t *request, fr_state_signal_t action, int limit)
 {
+       unlang_stack_frame_t    *frame;
        unlang_stack_t          *stack = request->stack;
+       int                     i, depth = stack->depth;
+
+       (void)talloc_get_type_abort(request, request_t);        /* Check the request hasn't already been freed */
+
+       fr_assert(stack->depth > 0);
 
-       switch (action) {
        /*
-        *      If we're stopping, then mark the request as stopped.
-        *      Then, call the frame signal handler.
+        *      Destructive signal where we clean each of the
+        *      stack frames up in turn.
+        *
+        *      We do this to avoid possible free ordering
+        *      issues where memory allocated by modules higher
+        *      in the stack is used by modules lower in the
+        *      stack.
         */
-       case FR_SIGNAL_CANCEL:
-               request->master_state = REQUEST_STOP_PROCESSING;
-               break;
+       if (action == FR_SIGNAL_CANCEL) {
+               for (i = depth; i > limit; i--) {
+                       frame = &stack->frame[i];
+                       if (frame->signal) frame->signal(request, frame, action);
+                       frame_pop(request->stack);
+               }
+               return;
+       }
+
+       /*
+        *      Walk back up the stack, calling signal handlers
+        *      to cancel any pending operations and free/release
+        *      any resources.
+        *
+        *      There may be multiple resumption points in the
+        *      stack, as modules can push xlats and function
+        *      calls.
+        */
+       for (i = depth; i > limit; i--) {
+               frame = &stack->frame[i];
+               if (frame->signal) frame->signal(request, frame, action);
+       }
+}
 
+/** Send a signal (usually stop) to a request
+ *
+ * This is typically called via an "async" action, i.e. an action
+ * outside of the normal processing of the request.
+ *
+ * If there is no #unlang_module_signal_t callback defined, the action is ignored.
+ *
+ * @param[in] request          The current request.
+ * @param[in] action           to signal.
+ */
+void unlang_interpret_signal(request_t *request, fr_state_signal_t action)
+{
+       unlang_stack_t          *stack = request->stack;
+
+       switch (action) {
        case FR_SIGNAL_DETACH:
                /*
                 *      Ensure the request is able to be detached
@@ -930,7 +928,7 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action)
        }
 
        /*
-        *      Requests that haven't been run through the interpret
+        *      Requests that haven't been run through the interpreter
         *      yet should have a stack depth of zero, so we don't
         *      need to do anything.
         */