From: Alan T. DeKok <aland@freeradius.org>
Date: Wed, 25 Dec 2024 22:08:18 +0000 (-0500)
Subject: unify bio_result_t and bio_request_t
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6ca0c37475c9beb9b32e98d68f041fbb1d3a940f;p=thirdparty%2Ffreeradius-server.git

unify bio_result_t and bio_request_t

they have the same lifetime, so there is no need for them to be
separate data structures.
---

diff --git a/src/modules/rlm_radius/bio.c b/src/modules/rlm_radius/bio.c
index a2b59d89623..edd97eced25 100644
--- a/src/modules/rlm_radius/bio.c
+++ b/src/modules/rlm_radius/bio.c
@@ -58,12 +58,7 @@ typedef struct {
 	};
 } bio_thread_t;
 
-typedef struct {
-	trunk_request_t		*treq;
-	rlm_rcode_t		rcode;			//!< from the transport
-	bool			is_retry;
-} bio_result_t;
-
+typedef struct bio_result_s bio_result_t;
 typedef struct bio_request_s bio_request_t;
 
 /** Track the handle, which is tightly correlated with the FD
@@ -136,6 +131,14 @@ struct bio_request_s {
 	fr_retry_t		retry;			//!< retransmission timers
 };
 
+typedef struct bio_result_s {
+	trunk_request_t		*treq;
+	rlm_rcode_t		rcode;			//!< from the transport
+	bool			is_retry;
+
+	bio_request_t		u;
+} bio_result_t;
+
 /** Turn a reply code into a module rcode;
  *
  */
@@ -158,7 +161,7 @@ static rlm_rcode_t radius_code_to_rcode[FR_RADIUS_CODE_MAX] = {
 static void		conn_writable_status_check(UNUSED fr_event_list_t *el, UNUSED int fd,
 						   UNUSED int flags, void *uctx);
 
-static int 		encode(rlm_radius_t const *inst, request_t *request, bio_request_t *u, uint8_t id);
+static int 		encode(bio_handle_t *h, request_t *request, bio_request_t *u, uint8_t id);
 
 static fr_radius_decode_fail_t	decode(TALLOC_CTX *ctx, fr_pair_list_t *reply, uint8_t *response_code,
 			       bio_handle_t *h, request_t *request, bio_request_t *u,
@@ -171,6 +174,8 @@ static unlang_action_t mod_resume(rlm_rcode_t *p_result, module_ctx_t const *mct
 static void mod_signal(module_ctx_t const *mctx, UNUSED request_t *request, fr_signal_t action);
 static void mod_write(request_t *request, trunk_request_t *treq, bio_handle_t *h);
 
+static int _bio_result_free(bio_result_t *r);
+
 #ifndef NDEBUG
 /** Log additional information about a tracking entry
  *
@@ -209,6 +214,8 @@ static void bio_request_reset(bio_request_t *u)
 	 *	if this is part of a pre-trunk status check.
 	 */
 	if (u->rr) radius_track_entry_release(&u->rr);
+
+	fr_assert(!u->ev);
 }
 
 /** Reset a status_check packet, ready to reuse
@@ -233,6 +240,7 @@ static void status_check_reset(bio_handle_t *h, bio_request_t *u)
  */
 static void CC_HINT(nonnull) status_check_alloc(bio_handle_t *h)
 {
+	bio_result_t		*r;
 	bio_request_t		*u;
 	request_t		*request;
 	rlm_radius_t const	*inst = h->inst;
@@ -240,7 +248,15 @@ static void CC_HINT(nonnull) status_check_alloc(bio_handle_t *h)
 
 	fr_assert(!h->status_u && !h->status_r && !h->status_request);
 
-	u = talloc_zero(h, bio_request_t);
+	MEM(request = request_local_alloc_external(h, NULL));
+	MEM(r = talloc_zero(request, bio_result_t));
+	talloc_set_destructor(r, _bio_result_free);
+
+	h->status_r = r;
+	u = &r->u;
+	h->status_u = u;
+
+	h->status_request = request;
 	fr_pair_list_init(&u->extra);
 
 	/*
@@ -257,7 +273,6 @@ static void CC_HINT(nonnull) status_check_alloc(bio_handle_t *h)
 	 *	head before the module destructor
 	 *      runs.
 	 */
-	request = request_local_alloc_external(u, NULL);
 	request->async = talloc_zero(request, fr_async_t);
 	talloc_const_free(request->name);
 	request->name = talloc_strdup(request, h->module_name);
@@ -324,10 +339,6 @@ static void CC_HINT(nonnull) status_check_alloc(bio_handle_t *h)
 
 	DEBUG3("%s - Status check packet type will be %s", h->module_name, fr_radius_packet_name[u->code]);
 	log_request_pair_list(L_DBG_LVL_3, request, NULL, &request->request_pairs, NULL);
-
-	MEM(h->status_r = talloc_zero(request, bio_result_t));
-	h->status_u = u;
-	h->status_request = request;
 }
 
 /** Connection errored
@@ -556,7 +567,7 @@ static void conn_writable_status_check(fr_event_list_t *el, UNUSED int fd, UNUSE
 	DEBUG("%s - Sending %s ID %d over connection %s",
 	      h->module_name, fr_radius_packet_name[u->code], u->id, h->fd_info->name);
 
-	if (encode(h->inst, h->status_request, u, u->id) < 0) {
+	if (encode(h, h->status_request, u, u->id) < 0) {
 	fail:
 		connection_signal_reconnect(conn, CONNECTION_FAILED);
 		return;
@@ -587,7 +598,7 @@ static void conn_writable_status_check(fr_event_list_t *el, UNUSED int fd, UNUSE
 	      h->module_name, (u->retry.count == 1) ? "Originated" : "Retransmitted",
 	      fr_box_time_delta(u->retry.rt));
 
-	if (fr_event_timer_at(u, el, &u->ev, u->retry.next, conn_status_check_timeout, conn) < 0) {
+	if (fr_event_timer_at(h, el, &u->ev, u->retry.next, conn_status_check_timeout, conn) < 0) {
 		PERROR("%s - Failed inserting timer event", h->module_name);
 		goto fail;
 	}
@@ -1135,10 +1146,12 @@ static fr_radius_decode_fail_t decode(TALLOC_CTX *ctx, fr_pair_list_t *reply, ui
 	return DECODE_FAIL_NONE;
 }
 
-static int encode(rlm_radius_t const *inst, request_t *request, bio_request_t *u, uint8_t id)
+static int encode(bio_handle_t *h, request_t *request, bio_request_t *u, uint8_t id)
 {
 	ssize_t			packet_len;
 	fr_radius_encode_ctx_t	encode_ctx;
+	bio_result_t		*r;
+	rlm_radius_t const	*inst = h->inst;
 
 	fr_assert(inst->allowed[u->code]);
 	fr_assert(!u->packet);
@@ -1147,8 +1160,11 @@ static int encode(rlm_radius_t const *inst, request_t *request, bio_request_t *u
 	 *	This is essentially free, as this memory was
 	 *	pre-allocated as part of the treq.
 	 */
+	r = (bio_result_t *)(((uintptr_t) u) - offsetof(bio_result_t, u));
+	(void) talloc_get_type_abort(r, bio_result_t);
+
 	u->packet_len = inst->max_packet_size;
-	MEM(u->packet = talloc_array(u, uint8_t, u->packet_len));
+	u->packet = h->buffer;
 
 	/*
 	 *	We should have at minimum 64-byte packets, so don't
@@ -1229,6 +1245,7 @@ static int encode(rlm_radius_t const *inst, request_t *request, bio_request_t *u
 		MEM(vp = fr_pair_afrom_da(u->packet, attr_proxy_state));
 		fr_pair_value_memdup(vp, (uint8_t const *) &inst->common_ctx.proxy_state, sizeof(inst->common_ctx.proxy_state), false);
 		fr_pair_append(&u->extra, vp);
+		packet_len += 6;
 	}
 
 	/*
@@ -1239,12 +1256,19 @@ static int encode(rlm_radius_t const *inst, request_t *request, bio_request_t *u
 	/*
 	 *	Now that we're done mangling the packet, sign it.
 	 */
-	if (fr_radius_sign(u->packet, NULL, (uint8_t const *) inst->secret,
-			   talloc_array_length(inst->secret) - 1) < 0) {
+	if (fr_radius_sign(u->packet, NULL, (uint8_t const *) inst->common_ctx.secret,
+			   inst->common_ctx.secret_length) < 0) {
 		RERROR("Failed signing packet");
 		goto error;
 	}
 
+	/*
+	 *	Keep a copy of the packet for potential retransmission.
+	 *
+	 *	@todo - do this only for UDP.
+	 */
+	MEM(u->packet = talloc_memdup(r, h->buffer, packet_len));
+
 	return 0;
 }
 
@@ -1393,12 +1417,13 @@ static void mod_retry(module_ctx_t const *mctx, request_t *request, fr_retry_t c
 	trunk_request_t		*treq = talloc_get_type_abort(r->treq, trunk_request_t);
 	trunk_connection_t	*tconn = treq->tconn;
 
-	bio_request_t		*u = talloc_get_type_abort(treq->preq, bio_request_t);
+	bio_request_t		*u = treq->preq;
 
 	bio_handle_t		*h;
 
 	fr_assert(request == treq->request);
 	fr_assert(treq->preq);						/* Must still have a protocol request */
+	fr_assert(u = &r->u);
 
 	switch (retry->state) {
 	case FR_RETRY_CONTINUE:
@@ -1499,7 +1524,7 @@ static void mod_write(request_t *request, trunk_request_t *treq, bio_handle_t *h
 	fr_assert((treq->state == TRUNK_REQUEST_STATE_PENDING) ||
 		  (treq->state == TRUNK_REQUEST_STATE_PARTIAL));
 
-	u = talloc_get_type_abort(treq->preq, bio_request_t);
+	u = treq->preq;
 
 	fr_assert(!u->status_check);
 
@@ -1540,13 +1565,12 @@ static void mod_write(request_t *request, trunk_request_t *treq, bio_handle_t *h
 		RDEBUG("Sending %s ID %d length %zu over connection %s",
 		       fr_radius_packet_name[u->code], u->id, u->packet_len, h->fd_info->name);
 
-		if (encode(h->inst, request, u, u->id) < 0) {
+		if (encode(h, request, u, u->id) < 0) {
 			/*
 			 *	Need to do this because request_conn_release
 			 *	may not be called.
 			 */
 			bio_request_reset(u);
-			if (u->ev) (void) fr_event_timer_delete(&u->ev);
 			trunk_request_signal_fail(treq);
 			return;
 		}
@@ -1830,7 +1854,7 @@ static void status_check_reply(trunk_request_t *treq, fr_time_t now)
 {
 	bio_handle_t		*h = talloc_get_type_abort(treq->tconn->conn->h, bio_handle_t);
 	rlm_radius_t const 	*inst = h->inst;
-	bio_request_t		*u = talloc_get_type_abort(treq->preq, bio_request_t);
+	bio_request_t		*u = treq->preq;
 	bio_result_t		*r = talloc_get_type_abort(treq->rctx, bio_result_t);
 
 	fr_assert(treq->preq == h->status_u);
@@ -1935,7 +1959,7 @@ static void request_demux(UNUSED fr_event_list_t *el, trunk_connection_t *tconn,
 		treq = talloc_get_type_abort(rr->uctx, trunk_request_t);
 		request = treq->request;
 		fr_assert(request != NULL);
-		u = talloc_get_type_abort(treq->preq, bio_request_t);
+		u = treq->preq;
 		r = talloc_get_type_abort(treq->rctx, bio_result_t);
 
 		/*
@@ -2034,7 +2058,7 @@ static void request_demux(UNUSED fr_event_list_t *el, trunk_connection_t *tconn,
 static void request_cancel(UNUSED connection_t *conn, void *preq_to_reset,
 			   trunk_cancel_reason_t reason, UNUSED void *uctx)
 {
-	bio_request_t	*u = talloc_get_type_abort(preq_to_reset, bio_request_t);
+	bio_request_t	*u = preq_to_reset;
 
 	/*
 	 *	Request has been requeued on the same
@@ -2065,7 +2089,7 @@ static void request_cancel(UNUSED connection_t *conn, void *preq_to_reset,
  */
 static void request_conn_release(connection_t *conn, void *preq_to_reset, UNUSED void *uctx)
 {
-	bio_request_t		*u = talloc_get_type_abort(preq_to_reset, bio_request_t);
+	bio_request_t		*u = preq_to_reset;
 	bio_handle_t		*h = talloc_get_type_abort(conn->h, bio_handle_t);
 
 	if (u->ev) (void)fr_event_timer_delete(&u->ev);
@@ -2089,7 +2113,7 @@ static void request_fail(request_t *request, void *preq, void *rctx,
 			 NDEBUG_UNUSED trunk_request_state_t state, UNUSED void *uctx)
 {
 	bio_result_t		*r = talloc_get_type_abort(rctx, bio_result_t);
-	bio_request_t		*u = talloc_get_type_abort(preq, bio_request_t);
+	bio_request_t		*u = preq;
 
 	fr_assert(!u->rr && !u->packet && fr_pair_list_empty(&u->extra) && !u->ev);	/* Dealt with by request_conn_release */
 
@@ -2109,7 +2133,7 @@ static void request_fail(request_t *request, void *preq, void *rctx,
 static void request_complete(request_t *request, void *preq, void *rctx, UNUSED void *uctx)
 {
 	bio_result_t		*r = talloc_get_type_abort(rctx, bio_result_t);
-	bio_request_t		*u = talloc_get_type_abort(preq, bio_request_t);
+	bio_request_t		*u = preq;
 
 	fr_assert(!u->rr && !u->packet && fr_pair_list_empty(&u->extra) && !u->ev);	/* Dealt with by request_conn_release */
 
@@ -2120,23 +2144,6 @@ static void request_complete(request_t *request, void *preq, void *rctx, UNUSED
 	unlang_interpret_mark_runnable(request);
 }
 
-/** Explicitly free resources associated with the protocol request
- *
- */
-static void request_free(UNUSED request_t *request, void *preq_to_free, UNUSED void *uctx)
-{
-	bio_request_t		*u = talloc_get_type_abort(preq_to_free, bio_request_t);
-
-	fr_assert(!u->rr && !u->packet && fr_pair_list_empty(&u->extra) && !u->ev);	/* Dealt with by request_conn_release */
-
-	/*
-	 *	Don't free status check requests.
-	 */
-	if (u->status_check) return;
-
-	talloc_free(u);
-}
-
 /** Resume execution of the request, returning the rcode set during trunk execution
  *
  */
@@ -2220,7 +2227,6 @@ static void mod_signal(module_ctx_t const *mctx, UNUSED request_t *request, fr_s
 	}
 }
 
-#ifndef NDEBUG
 /** Free a bio_result_t
  *
  * Allows us to set break points for debugging.
@@ -2233,18 +2239,11 @@ static int _bio_result_free(bio_result_t *r)
 	if (!r->treq) return 0;
 
 	treq = talloc_get_type_abort(r->treq, trunk_request_t);
-	u = talloc_get_type_abort(treq->preq, bio_request_t);
+	u = treq->preq;
+	fr_assert(u == &r->u);
 
 	fr_assert_msg(!u->ev, "bio_result_t freed with active timer");
 
-	return 0;
-}
-#endif
-
-/** Free a bio_request_t
- */
-static int _bio_request_free(bio_request_t *u)
-{
 	if (u->ev) (void) fr_event_timer_delete(&u->ev);
 
 	fr_assert(u->rr == NULL);
@@ -2272,14 +2271,12 @@ static unlang_action_t mod_enqueue(rlm_rcode_t *p_result, rlm_radius_t const *in
 	if (!treq) RETURN_MODULE_FAIL;
 
 	MEM(r = talloc_zero(request, bio_result_t));
-#ifndef NDEBUG
 	talloc_set_destructor(r, _bio_result_free);
-#endif
 
 	/*
 	 *	Can't use compound literal - const issues.
 	 */
-	MEM(u = talloc_zero(treq, bio_request_t));
+	u = &r->u;
 	u->code = request->packet->code;
 	u->priority = request->async->priority;
 	u->recv_time = request->async->recv_time;
@@ -2314,8 +2311,6 @@ static unlang_action_t mod_enqueue(rlm_rcode_t *p_result, rlm_radius_t const *in
 	r->treq = treq;	/* Remember for signalling purposes */
 	fr_assert(treq->rctx == r);
 
-	talloc_set_destructor(u, _bio_request_free);
-
 	/*
 	 *	Figure out if we're originating the packet or proxying it.  And also figure out if we have to
 	 *	retry.
@@ -2400,7 +2395,6 @@ static int mod_thread_instantiate(module_thread_inst_ctx_t const *mctx)
 						.request_complete = request_complete,
 						.request_fail = request_fail,
 						.request_cancel = request_cancel,
-						.request_free = request_free
 					};
 
 	thread->el = mctx->el;