From: Alan T. DeKok Date: Wed, 6 Aug 2025 15:43:43 +0000 (-0400) Subject: revisit and clean up destructor vs shutdown X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=013660fb0e2859029e906012d7dbf51e387999b8;p=thirdparty%2Ffreeradius-server.git revisit and clean up destructor vs shutdown shutdown can be called on fatal error, and only stops the BIO. the underlying BIO is still there. This allows it to be called from a BIO which is in the middle of a chain. destructor calls shutdown first, and then frees the resources. this allows a destructor to be called from anywhere, and then the entire chain is shut down --- diff --git a/src/lib/bio/base.c b/src/lib/bio/base.c index 05b217ba4e..9b07726097 100644 --- a/src/lib/bio/base.c +++ b/src/lib/bio/base.c @@ -26,22 +26,20 @@ #include #include -#ifndef NDEBUG /** Free this bio. * - * The bio can only be freed if it is not in any chain. + * We allow talloc_free() to be called on just about anything in the + * bio chain. But we ensure that the chain is always shut down in an + * orderly fashion. */ int fr_bio_destructor(fr_bio_t *bio) { - fr_assert(!fr_bio_prev(bio)); - fr_assert(!fr_bio_next(bio)); + fr_bio_common_t *my = (fr_bio_common_t *) bio; + + FR_BIO_DESTRUCTOR_COMMON; - /* - * It's safe to free this bio. - */ return 0; } -#endif /** Internal bio function which just reads from the "next" bio. * @@ -89,50 +87,6 @@ ssize_t fr_bio_next_write(fr_bio_t *bio, void *packet_ctx, void const *buffer, s return rcode; } -/** Free this bio, and everything it calls. - * - * We unlink the bio chain, and then free it individually. If there's an error, the bio chain is relinked. - * That way the error can be addressed (somehow) and this function can be called again. - * - * Note that we do not support talloc_free() for the bio chain. Each individual bio has to be unlinked from - * the chain before the destructor will allow it to be freed. This functionality is by design. - * - * We want to have an API where bios are created "bottom up", so that it is impossible for an application to - * create an incorrect chain. However, creating the chain bottom up means that the lower bios not parented - * from the higher bios, and therefore talloc_free() won't free them. As a result, we need an explicit - * bio_free() function. - */ -int fr_bio_free(fr_bio_t *bio) -{ - fr_bio_t *next = fr_bio_next(bio); - - /* - * We cannot free a bio in the middle of a chain. It has to be unlinked first. - */ - if (fr_bio_prev(bio)) return -1; - - /* - * Unlink our bio, and recurse to free the next one. If we can't free it, re-chain it, but reset - * the read/write functions to do nothing. - */ - if (next) { - next->entry.prev = NULL; - if (fr_bio_free(next) < 0) { - next->entry.prev = &bio->entry; - bio->read = fr_bio_fail_read; - bio->write = fr_bio_fail_write; - return -1; - } - - bio->entry.next = NULL; - } - - /* - * It's now safe to free this bio. - */ - return talloc_free(bio); -} - static ssize_t fr_bio_shutdown_read(UNUSED fr_bio_t *bio, UNUSED void *packet_ctx, UNUSED void *buffer, UNUSED size_t size) { return fr_bio_error(SHUTDOWN); @@ -145,8 +99,17 @@ static ssize_t fr_bio_shutdown_write(UNUSED fr_bio_t *bio, UNUSED void *packet_c /** Shut down a set of BIOs * - * We shut down the BIOs from the top to the bottom. This gives the TLS BIO an opportunity to - * call the SSL_shutdown() routine, which should then write to the FD BIO. + * We shut down the BIOs from the top to the bottom. This gives the + * TLS BIO an opportunity to call the SSL_shutdown() routine, which + * should then write to the FD BIO. Once that write is completed, + * the FD BIO can then close its socket. + * + * Any shutdown is "stop read / write", but is not "free all + * resources". A shutdown can happen when one of the intermediary + * BIOs hits a fatal error. It can't free the BIO, but it has to + * mark the entire BIO chain as being unusable. + * + * A destructor will first shutdown the BIOs, and then free all resources. */ int fr_bio_shutdown(fr_bio_t *bio) { @@ -186,7 +149,8 @@ int fr_bio_shutdown(fr_bio_t *bio) } /* - * Call the application shutdown routine + * Call the application shutdown routine to tell it that + * the BIO has been successfully shut down. */ my = (fr_bio_common_t *) first; diff --git a/src/lib/bio/base.h b/src/lib/bio/base.h index 1d08a38899..4d83f920ea 100644 --- a/src/lib/bio/base.h +++ b/src/lib/bio/base.h @@ -184,18 +184,12 @@ static inline ssize_t CC_HINT(nonnull(1)) fr_bio_write(fr_bio_t *bio, void *pack int fr_bio_shutdown_intermediate(fr_bio_t *bio) CC_HINT(nonnull); -#ifndef NDEBUG -int fr_bio_destructor(fr_bio_t *bio) CC_HINT(nonnull); -#else -#define fr_bio_destructor (NULL) -#endif +int fr_bio_destructor(fr_bio_t *bio); #define fr_bio_error(_x) (-(FR_BIO_ERROR_ ## _x)) int fr_bio_shutdown(fr_bio_t *bio) CC_HINT(nonnull); -int fr_bio_free(fr_bio_t *bio) CC_HINT(nonnull); - char const *fr_bio_strerror(ssize_t error); void fr_bio_cb_set(fr_bio_t *bio, fr_bio_cb_funcs_t const *cb) CC_HINT(nonnull(1)); diff --git a/src/lib/bio/bio_priv.h b/src/lib/bio/bio_priv.h index 4c42a3216c..706b016270 100644 --- a/src/lib/bio/bio_priv.h +++ b/src/lib/bio/bio_priv.h @@ -56,6 +56,22 @@ struct fr_bio_common_s { FR_BIO_COMMON; }; +/** Define a common destructor pattern. + * + * Ensure that talloc_free() is safe no matter what. The caller can free any BIO at any time. If that + * happens, then the entire chain is shut down. On successful shutdown, this BIO is removed from the chain. + */ +#define FR_BIO_DESTRUCTOR_COMMON \ +do { \ + if (my->priv_cb.shutdown) { \ + int rcode; \ + rcode = fr_bio_shutdown(&my->bio); \ + if (rcode < 0) return rcode; \ + } \ + fr_bio_unchain(&my->bio); \ +} while (0) + + ssize_t fr_bio_next_read(fr_bio_t *bio, void *packet_ctx, void *buffer, size_t size); ssize_t fr_bio_next_write(fr_bio_t *bio, void *packet_ctx, void const *buffer, size_t size); diff --git a/src/lib/bio/dedup.c b/src/lib/bio/dedup.c index badd252e5a..252e520852 100644 --- a/src/lib/bio/dedup.c +++ b/src/lib/bio/dedup.c @@ -1006,10 +1006,11 @@ int fr_bio_dedup_entry_extend(fr_bio_t *bio, fr_bio_dedup_entry_t *item, fr_time /** Remove the dedup cache * */ -static int fr_bio_dedup_destructor(fr_bio_dedup_t *my) +static int fr_bio_dedup_shutdown(fr_bio_t *bio) { fr_rb_iter_inorder_t iter; fr_bio_dedup_entry_t *item; + fr_bio_dedup_t *my = talloc_get_type_abort(bio, fr_bio_dedup_t); talloc_const_free(my->ev); @@ -1090,7 +1091,9 @@ fr_bio_t *fr_bio_dedup_alloc(TALLOC_CTX *ctx, size_t max_saved, fr_bio_chain(&my->bio, next); - talloc_set_destructor(my, fr_bio_dedup_destructor); + my->priv_cb.shutdown = fr_bio_dedup_shutdown; + + talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); /* always use a common destructor */ return (fr_bio_t *) my; } diff --git a/src/lib/bio/fd.c b/src/lib/bio/fd.c index 813f152df4..8b12dfdf76 100644 --- a/src/lib/bio/fd.c +++ b/src/lib/bio/fd.c @@ -106,11 +106,6 @@ static int fr_bio_fd_shutdown(fr_bio_t *bio) return fr_bio_fd_close(&my->bio); } -static int fr_bio_fd_destructor(fr_bio_fd_t *my) -{ - return fr_bio_fd_shutdown((fr_bio_t *) my); -} - static int fr_bio_fd_eof(fr_bio_t *bio) { fr_bio_fd_t *my = talloc_get_type_abort(bio, fr_bio_fd_t); @@ -712,7 +707,7 @@ static ssize_t fr_bio_fd_try_connect(fr_bio_fd_t *my) } if (rcode < 0) { - fr_bio_shutdown(&my->bio); + (void) fr_bio_shutdown(&my->bio); return fr_bio_error(GENERIC); } @@ -762,7 +757,7 @@ retry: } fail: - fr_bio_shutdown(&my->bio); + (void) fr_bio_shutdown(&my->bio); return fr_bio_error(IO); } @@ -998,7 +993,7 @@ fr_bio_t *fr_bio_fd_alloc(TALLOC_CTX *ctx, fr_bio_fd_config_t const *cfg, size_t my->priv_cb.write_resume = fr_bio_fd_write_resume; my->priv_cb.shutdown = fr_bio_fd_shutdown; - talloc_set_destructor(my, fr_bio_fd_destructor); + talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); /* always use a common destructor */ return (fr_bio_t *) my; } @@ -1071,7 +1066,10 @@ static void fr_bio_fd_el_error(UNUSED fr_event_list_t *el, UNUSED int fd, UNUSED my->connect.error(&my->bio); } - fr_bio_shutdown(&my->bio); + /* + * The entire bio is unusable. + */ + (void) fr_bio_shutdown(&my->bio); } /** Connect callback for when the socket is writable. @@ -1154,7 +1152,7 @@ static void fr_bio_fd_el_timeout(UNUSED fr_timer_list_t *tl, UNUSED fr_time_t no my->connect.timeout(&my->bio); - fr_bio_shutdown(&my->bio); + (void) fr_bio_shutdown(&my->bio); } @@ -1191,7 +1189,7 @@ int fr_bio_fd_connect_full(fr_bio_t *bio, fr_event_list_t *el, fr_bio_callback_t my->info.connect_errno = ECONNREFUSED; #endif if (error_cb) error_cb(bio); - fr_bio_shutdown(&my->bio); + (void) fr_bio_shutdown(&my->bio); return fr_bio_error(GENERIC); } diff --git a/src/lib/bio/fd_errno.h b/src/lib/bio/fd_errno.h index 72217af811..7ee8defae9 100644 --- a/src/lib/bio/fd_errno.h +++ b/src/lib/bio/fd_errno.h @@ -67,4 +67,4 @@ default: /* * Shut down the BIO. It's no longer useable. */ -fr_bio_shutdown(&my->bio); +(void) fr_bio_shutdown(&my->bio); diff --git a/src/lib/bio/haproxy.c b/src/lib/bio/haproxy.c index 13be88c2cd..696349e616 100644 --- a/src/lib/bio/haproxy.c +++ b/src/lib/bio/haproxy.c @@ -62,7 +62,7 @@ static ssize_t fr_bio_haproxy_v1(fr_bio_haproxy_t *my) */ if (memcmp(my->buffer.read, "PROXY TCP", 9) != 0) { fail: - fr_bio_shutdown(&my->bio); + (void) fr_bio_shutdown(&my->bio); return fr_bio_error(VERIFY); } p += 9; @@ -239,7 +239,7 @@ fr_bio_t *fr_bio_haproxy_alloc(TALLOC_CTX *ctx, fr_bio_cb_funcs_t *cb, fr_bio_t fr_bio_chain(&my->bio, next); - talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); + talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); /* always use a common destructor */ return (fr_bio_t *) my; } diff --git a/src/lib/bio/mem.c b/src/lib/bio/mem.c index 5829d485b2..2d3d71ae2e 100644 --- a/src/lib/bio/mem.c +++ b/src/lib/bio/mem.c @@ -350,7 +350,7 @@ static ssize_t fr_bio_mem_read_verify_datagram(fr_bio_t *bio, void *packet_ctx, break; } - fr_bio_shutdown(bio); + (void) fr_bio_shutdown(bio); return fr_bio_error(VERIFY); } @@ -679,7 +679,7 @@ static int fr_bio_mem_call_verify(fr_bio_t *bio, void *packet_ctx, size_t *size) /* * A fatal error. Shut down the entire BIO chain. */ - fr_bio_shutdown(bio); + (void) fr_bio_shutdown(bio); return -1; } @@ -779,7 +779,7 @@ fr_bio_t *fr_bio_mem_alloc(TALLOC_CTX *ctx, size_t read_size, size_t write_size, fr_bio_chain(&my->bio, next); - talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); + talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); /* always use a common destructor */ return (fr_bio_t *) my; } @@ -883,7 +883,7 @@ fr_bio_t *fr_bio_mem_sink_alloc(TALLOC_CTX *ctx, size_t read_size) my->bio.read = fr_bio_mem_read_buffer; my->bio.write = fr_bio_mem_write_read_buffer; /* the upstream will write to our read buffer */ - talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); + talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); /* always use a common destructor */ return (fr_bio_t *) my; } diff --git a/src/lib/bio/packet.c b/src/lib/bio/packet.c index b0ece95e62..f6aa7f621b 100644 --- a/src/lib/bio/packet.c +++ b/src/lib/bio/packet.c @@ -151,8 +151,12 @@ void fr_bio_packet_connected(fr_bio_t *bio) static int fr_bio_packet_shutdown(fr_bio_t *bio) { + int rcode; fr_bio_packet_t *my = bio->uctx; + rcode = fr_bio_shutdown(bio); + if (rcode < 0) return rcode; + if (!my->cb.shutdown) return 0; return my->cb.shutdown(my); diff --git a/src/lib/bio/pipe.c b/src/lib/bio/pipe.c index ccfd7f0422..f8df91302e 100644 --- a/src/lib/bio/pipe.c +++ b/src/lib/bio/pipe.c @@ -47,6 +47,8 @@ typedef struct { static int fr_bio_pipe_destructor(fr_bio_pipe_t *my) { + FR_BIO_DESTRUCTOR_COMMON; + pthread_mutex_destroy(&my->mutex); return 0; diff --git a/src/lib/bio/queue.c b/src/lib/bio/queue.c index f6aa6c3cce..d341b2177c 100644 --- a/src/lib/bio/queue.c +++ b/src/lib/bio/queue.c @@ -100,6 +100,8 @@ static void fr_bio_queue_list_cancel(fr_bio_queue_t *my) static int fr_bio_queue_destructor(fr_bio_queue_t *my) { + FR_BIO_DESTRUCTOR_COMMON; + fr_assert(my->cancel); /* otherwise it would be fr_bio_destructor */ fr_bio_queue_list_cancel(my); diff --git a/src/lib/bio/retry.c b/src/lib/bio/retry.c index 1e85472538..9d4bfa5357 100644 --- a/src/lib/bio/retry.c +++ b/src/lib/bio/retry.c @@ -885,11 +885,6 @@ static int fr_bio_retry_shutdown(fr_bio_t *bio) return 0; } -static int fr_bio_retry_destructor(fr_bio_retry_t *my) -{ - return fr_bio_retry_shutdown((fr_bio_t *) my); -} - /** Allocate a #fr_bio_retry_t * */ @@ -978,8 +973,7 @@ fr_bio_t *fr_bio_retry_alloc(TALLOC_CTX *ctx, size_t max_saved, fr_bio_chain(&my->bio, next); - talloc_set_destructor(my, fr_bio_retry_destructor); - + talloc_set_destructor((fr_bio_t *) my, fr_bio_destructor); /* always use a common destructor */ return (fr_bio_t *) my; } diff --git a/src/modules/rlm_radius/bio.c b/src/modules/rlm_radius/bio.c index 01e0ab3e7e..c9c6c4654e 100644 --- a/src/modules/rlm_radius/bio.c +++ b/src/modules/rlm_radius/bio.c @@ -896,8 +896,6 @@ static void conn_close(UNUSED fr_event_list_t *el, void *handle, fr_assert_fail("%u tracking entries still allocated at conn close", h->tt->num_requests); } - fr_bio_shutdown(h->bio.mem); - /* * We have opened a limited number of outbound source ports. This means that when we close a * port, we have to mark it unused. diff --git a/src/protocols/radius/client.c b/src/protocols/radius/client.c index 79bc5c3735..33e7a9a90c 100644 --- a/src/protocols/radius/client.c +++ b/src/protocols/radius/client.c @@ -61,16 +61,6 @@ fr_bio_packet_t *fr_radius_client_bio_alloc(TALLOC_CTX *ctx, fr_radius_client_co return fr_radius_client_tcp_bio_alloc(ctx, cfg, fd_cfg); } -static int _radius_client_fd_bio_free(fr_radius_client_fd_bio_t *my) -{ - if (fr_bio_shutdown(my->common.bio) < 0) return -1; - - if (fr_bio_free(my->common.bio) < 0) return -1; - - return 0; -} - - fr_radius_client_fd_bio_t *fr_radius_client_fd_bio_alloc(TALLOC_CTX *ctx, size_t read_size, fr_radius_client_config_t *cfg, fr_bio_fd_config_t const *fd_cfg) { int i; @@ -153,8 +143,6 @@ fr_radius_client_fd_bio_t *fr_radius_client_fd_bio_alloc(TALLOC_CTX *ctx, size_t */ fr_bio_packet_init(&my->common); - talloc_set_destructor(my, _radius_client_fd_bio_free); - /* * Set up the connected status. */ diff --git a/src/protocols/radius/server.c b/src/protocols/radius/server.c index 3e9122ec7f..be8f6ff985 100644 --- a/src/protocols/radius/server.c +++ b/src/protocols/radius/server.c @@ -45,16 +45,6 @@ fr_bio_packet_t *fr_radius_server_bio_alloc(TALLOC_CTX *ctx, fr_radius_server_co // return fr_radius_server_tcp_bio_alloc(ctx, cfg, fd_cfg); } -static int _radius_server_fd_bio_free(fr_radius_server_fd_bio_t *my) -{ - if (fr_bio_shutdown(my->common.bio) < 0) return -1; - - if (fr_bio_free(my->common.bio) < 0) return -1; - - return 0; -} - - fr_radius_server_fd_bio_t *fr_radius_server_fd_bio_alloc(TALLOC_CTX *ctx, size_t read_size, fr_radius_server_config_t *cfg, fr_bio_fd_config_t const *fd_cfg) { fr_radius_server_fd_bio_t *my; @@ -92,8 +82,6 @@ fr_radius_server_fd_bio_t *fr_radius_server_fd_bio_alloc(TALLOC_CTX *ctx, size_t my->common.bio = my->mem; - talloc_set_destructor(my, _radius_server_fd_bio_free); - return my; }