From: Arran Cudbard-Bell Date: Wed, 29 Mar 2023 18:25:58 +0000 (-0600) Subject: subrequests/synchronous interpreter: Use the detached done callback to free requests X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2fe849a6ea52a8ebc6e9126764fffe4a61bbb602;p=thirdparty%2Ffreeradius-server.git subrequests/synchronous interpreter: Use the detached done callback to free requests Use unlang_interpret_signal to manage cleaning up detached requests instead of having custom code inside the subrequest code --- diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index 06e69c5b9a4..fc23e36dc35 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -997,7 +997,7 @@ void unlang_interpret_request_done(request_t *request) break; case REQUEST_TYPE_DETACHED: - intp->funcs.done_detached(request, stack->result, intp->uctx); + intp->funcs.done_detached(request, stack->result, intp->uctx); /* Callback will usually free the request */ break; } } @@ -1024,8 +1024,12 @@ void unlang_interpret_request_detach(request_t *request) if (!fr_cond_assert(stack != NULL)) return; + if (!request_is_detachable(request)) return; + intp = stack->intp; intp->funcs.detach(request, intp->uctx); + + if (request_detach(request) < 0) RPEDEBUG("Failed detaching request"); } /** Send a signal (usually stop) to a request @@ -1123,22 +1127,33 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action) switch (action) { case FR_SIGNAL_CANCEL: - unlang_interpret_request_stop(request); /* Stop gets the request in a consistent state */ - unlang_interpret_request_done(request); /* Done signals the request is complete */ + /* + * Detach the request from the parent to cleanup + * any cross-request pointers. This is a noop + * if the request is not detachable. + */ + if (request_is_detachable(request)) unlang_interpret_request_detach(request); /* - * If we're cancelling a child, detach it from - * its parent, so we don't leak memory allocated - * in request_detachable_init. + * Get the request into a consistent state, + * removing it from any runnable lists. */ - if (request_is_detachable(request) && (request_detach(request) < 0)) { - RPEDEBUG("Failed detaching request on cancel"); - } + unlang_interpret_request_stop(request); + + /* + * As the request is detached, we call the done_detached + * callback which should free the request. + */ + unlang_interpret_request_done(request); break; case FR_SIGNAL_DETACH: - unlang_interpret_request_detach(request); /* Tell our caller that the request is being detached */ - if (request_detach(request) < 0) RPEDEBUG("Failed detaching request"); + /* + * Cleanup any cross-request pointers, and mark the + * request as detached. When the request completes it + * should by automatically freed. + */ + unlang_interpret_request_detach(request); break; default: diff --git a/src/lib/unlang/interpret_synchronous.c b/src/lib/unlang/interpret_synchronous.c index a8c881f8825..7df176e2597 100644 --- a/src/lib/unlang/interpret_synchronous.c +++ b/src/lib/unlang/interpret_synchronous.c @@ -84,6 +84,15 @@ static void _request_done_internal(request_t *request, UNUSED rlm_rcode_t rcode, static void _request_done_detached(request_t *request, UNUSED rlm_rcode_t rcode, UNUSED void *uctx) { RDEBUG3("Done synchronous detached request"); + + /* + * Detached requests have to be freed by us + * as nothing else can free them. + * + * All other requests must be freed by the + * code which allocated them. + */ + talloc_free(request); } /** We don't need to do anything for internal -> detached @@ -278,14 +287,11 @@ rlm_rcode_t unlang_interpret_synchronous(fr_event_list_t *el, request_t *request RDEBUG4("<<< interpreter (iteration %i) - %s", iterations, fr_table_str_by_value(rcode_table, sub_rcode, "")); - if (sub_request == request) { - rcode = sub_rcode; /* - * Free detached, resumable requests + * If the request being run was the original request + * then update the rcode we're returning. */ - } else if (!sub_request->parent && !unlang_interpret_is_resumable(sub_request)) { - talloc_free(sub_request); /* Free detached requests */ - } + if (sub_request == request) rcode = sub_rcode; DEBUG3("%u runnable, %u yielded", fr_heap_num_elements(intps->runnable), intps->yielded); } diff --git a/src/lib/unlang/subrequest_child.c b/src/lib/unlang/subrequest_child.c index 2186c7d7ad0..2ae15804e53 100644 --- a/src/lib/unlang/subrequest_child.c +++ b/src/lib/unlang/subrequest_child.c @@ -56,21 +56,7 @@ static void unlang_detached_max_request_time(UNUSED fr_event_list_t *el, UNUSED RDEBUG("Reached Request-Lifetime. Forcibly stopping request"); - fr_assert(!request->parent); /* must be detached */ - - /* - * The request is scheduled and isn't running. Remove it - * from the backlog. - */ - if (unlang_request_is_scheduled(request)) { - unlang_stack_t *stack = request->stack; - unlang_interpret_t *intp = stack->intp; - - fr_assert(request_is_internal(request)); - intp->funcs.done_internal(request, stack->result, intp->uctx); /* Should free the request */ - } else { - talloc_free(request); - } + unlang_interpret_signal(request, FR_SIGNAL_CANCEL); /* Request should now be freed */ } /** Initialize a detached child @@ -127,6 +113,12 @@ static void unlang_subrequest_child_signal(request_t *request, fr_state_signal_t { unlang_frame_state_subrequest_t *state; + /* + * We're already detached so we don't + * need to notify the parent we're + * waking up, and we don't need to detach + * again... + */ if (!request->parent) return; state = talloc_get_type_abort(frame_current(request->parent)->state, unlang_frame_state_subrequest_t); @@ -155,7 +147,7 @@ static void unlang_subrequest_child_signal(request_t *request, fr_state_signal_t * Indicate to the parent there's no longer a child */ state->child = NULL; - + /* * Tell the parent to resume */