From: Vsevolod Stakhov Date: Thu, 29 Jan 2026 15:16:59 +0000 (+0000) Subject: [Fix] hs_helper: fix use-after-free in async hyperscan cache callbacks X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=097beafc7e645909577eade8b202b3b701ee8e28;p=thirdparty%2Frspamd.git [Fix] hs_helper: fix use-after-free in async hyperscan cache callbacks Add proper refcounting to async compilation contexts to prevent use-after-free when Redis callbacks are invoked multiple times (timeout + response) or during worker termination. - Add ref_entry_t to async context structures - Use REF_RETAIN before async operations and REF_RELEASE in callbacks - Add callback_processed flag to prevent double processing - Save entry data before ev_run that might free pending arrays --- diff --git a/src/hs_helper.c b/src/hs_helper.c index 105b6a0f87..f6923a0285 100644 --- a/src/hs_helper.c +++ b/src/hs_helper.c @@ -16,6 +16,7 @@ #include "config.h" #include "libutil/util.h" #include "libutil/multipattern.h" +#include "libutil/ref.h" #include "libserver/cfg_file.h" #include "libserver/cfg_rcl.h" #include "libserver/worker_util.h" @@ -814,9 +815,24 @@ struct rspamd_hs_helper_mp_async_ctx { struct rspamd_multipattern_pending *pending; unsigned int count; unsigned int idx; + gboolean compile_cb_called; ev_timer deferred_timer; /* Timer for deferring next operation after error */ + ref_entry_t ref; }; +static void +rspamd_hs_helper_mp_async_ctx_dtor(void *p) +{ + struct rspamd_hs_helper_mp_async_ctx *mpctx = p; + + /* Stop the deferred timer if it's somehow still active */ + if (ev_is_active(&mpctx->deferred_timer)) { + ev_timer_stop(mpctx->ctx->event_loop, &mpctx->deferred_timer); + } + rspamd_multipattern_clear_pending(); + g_free(mpctx); +} + static void rspamd_hs_helper_mp_send_notification(struct hs_helper_ctx *ctx, struct rspamd_worker *worker, @@ -853,9 +869,17 @@ rspamd_hs_helper_mp_compiled_cb(struct rspamd_multipattern *mp, void *ud) { struct rspamd_hs_helper_mp_async_ctx *mpctx = ud; - struct rspamd_multipattern_pending *entry = &mpctx->pending[mpctx->idx]; + struct rspamd_multipattern_pending *entry; (void) mp; + + if (mpctx->compile_cb_called) { + REF_RELEASE(mpctx); + return; + } + mpctx->compile_cb_called = TRUE; + + entry = &mpctx->pending[mpctx->idx]; rspamd_worker_set_busy(mpctx->worker, mpctx->ctx->event_loop, NULL); if (!success) { @@ -876,6 +900,8 @@ rspamd_hs_helper_mp_compiled_cb(struct rspamd_multipattern *mp, mpctx->deferred_timer.data = mpctx; ev_timer_init(&mpctx->deferred_timer, rspamd_hs_helper_mp_deferred_next_cb, 0.0, 0.0); ev_timer_start(mpctx->ctx->event_loop, &mpctx->deferred_timer); + + REF_RELEASE(mpctx); } static void @@ -888,12 +914,19 @@ rspamd_hs_helper_mp_exists_cb(gboolean success, struct rspamd_hs_helper_mp_async_ctx *mpctx = ud; struct rspamd_multipattern_pending *entry = &mpctx->pending[mpctx->idx]; bool exists = (success && data == NULL && len == 1); + /* + * Save entry data before any operation that might trigger ev_run, + * as ev_run could process deferred timers that call + * rspamd_multipattern_clear_pending() and free the pending array. + */ + struct rspamd_multipattern *mp = entry->mp; + const char *entry_name = entry->name; (void) error; if (exists) { - msg_debug_hyperscan("multipattern cache already exists for '%s', skipping compilation", entry->name); - rspamd_hs_helper_mp_send_notification(mpctx->ctx, mpctx->worker, entry->name); + msg_debug_hyperscan("multipattern cache already exists for '%s', skipping compilation", entry_name); + rspamd_hs_helper_mp_send_notification(mpctx->ctx, mpctx->worker, entry_name); mpctx->idx++; /* * Defer the next operation to avoid nested Redis callbacks. @@ -903,6 +936,7 @@ rspamd_hs_helper_mp_exists_cb(gboolean success, mpctx->deferred_timer.data = mpctx; ev_timer_init(&mpctx->deferred_timer, rspamd_hs_helper_mp_deferred_next_cb, 0.0, 0.0); ev_timer_start(mpctx->ctx->event_loop, &mpctx->deferred_timer); + REF_RELEASE(mpctx); return; } @@ -910,7 +944,10 @@ rspamd_hs_helper_mp_exists_cb(gboolean success, rspamd_worker_set_busy(mpctx->worker, mpctx->ctx->event_loop, "compile multipattern"); /* Flush the busy notification before blocking on compilation */ ev_run(mpctx->ctx->event_loop, EVRUN_NOWAIT); - rspamd_multipattern_compile_hs_to_cache_async(entry->mp, mpctx->ctx->hs_dir, + /* Use saved mp pointer - pending array may have been freed by ev_run */ + mpctx->compile_cb_called = FALSE; + REF_RETAIN(mpctx); + rspamd_multipattern_compile_hs_to_cache_async(mp, mpctx->ctx->hs_dir, mpctx->ctx->event_loop, rspamd_hs_helper_mp_compiled_cb, mpctx); } @@ -935,6 +972,7 @@ rspamd_hs_helper_compile_pending_multipatterns_next(struct rspamd_hs_helper_mp_a char cache_key[rspamd_cryptobox_HASHBYTES * 2 + 1]; rspamd_snprintf(cache_key, sizeof(cache_key), "%*xs", (int) sizeof(entry->hash) / 2, entry->hash); + REF_RETAIN(mpctx); rspamd_hs_cache_lua_exists_async(cache_key, entry->name, rspamd_hs_helper_mp_exists_cb, mpctx); return; @@ -968,16 +1006,7 @@ rspamd_hs_helper_compile_pending_multipatterns_next(struct rspamd_hs_helper_mp_a } done: - /* Stop the deferred timer if it's somehow still active */ - if (ev_is_active(&mpctx->deferred_timer)) { - ev_timer_stop(mpctx->ctx->event_loop, &mpctx->deferred_timer); - } - rspamd_multipattern_clear_pending(); - for (unsigned int i = 0; i < mpctx->count; i++) { - /* names are freed by clear_pending */ - (void) i; - } - g_free(mpctx); + REF_RELEASE(mpctx); } static void @@ -1001,6 +1030,7 @@ rspamd_hs_helper_compile_pending_multipatterns(struct hs_helper_ctx *ctx, mpctx->pending = pending; mpctx->count = count; mpctx->idx = 0; + REF_INIT_RETAIN(mpctx, rspamd_hs_helper_mp_async_ctx_dtor); rspamd_hs_helper_compile_pending_multipatterns_next(mpctx); } @@ -1015,10 +1045,20 @@ struct rspamd_hs_helper_remap_async_ctx { struct rspamd_regexp_map_pending *pending; unsigned int count; unsigned int idx; + gboolean compile_cb_called; + ref_entry_t ref; }; static void rspamd_hs_helper_compile_pending_regexp_maps_next(struct rspamd_hs_helper_remap_async_ctx *rmctx); +static void +rspamd_hs_helper_remap_async_ctx_dtor(void *p) +{ + struct rspamd_hs_helper_remap_async_ctx *rmctx = p; + rspamd_regexp_map_clear_pending(); + g_free(rmctx); +} + static void rspamd_hs_helper_remap_send_notification(struct hs_helper_ctx *ctx, struct rspamd_worker *worker, @@ -1042,9 +1082,17 @@ rspamd_hs_helper_remap_compiled_cb(struct rspamd_regexp_map_helper *re_map, void *ud) { struct rspamd_hs_helper_remap_async_ctx *rmctx = ud; - struct rspamd_regexp_map_pending *entry = &rmctx->pending[rmctx->idx]; + struct rspamd_regexp_map_pending *entry; (void) re_map; + + if (rmctx->compile_cb_called) { + REF_RELEASE(rmctx); + return; + } + rmctx->compile_cb_called = TRUE; + + entry = &rmctx->pending[rmctx->idx]; rspamd_worker_set_busy(rmctx->worker, rmctx->ctx->event_loop, NULL); if (!success) { @@ -1056,6 +1104,7 @@ rspamd_hs_helper_remap_compiled_cb(struct rspamd_regexp_map_helper *re_map, rmctx->idx++; rspamd_hs_helper_compile_pending_regexp_maps_next(rmctx); + REF_RELEASE(rmctx); } static void @@ -1068,14 +1117,22 @@ rspamd_hs_helper_remap_exists_cb(gboolean success, struct rspamd_hs_helper_remap_async_ctx *rmctx = ud; struct rspamd_regexp_map_pending *entry = &rmctx->pending[rmctx->idx]; bool exists = (success && data == NULL && len == 1); + /* + * Save entry data before any operation that might trigger ev_run, + * as ev_run could process deferred timers that call + * rspamd_regexp_map_clear_pending() and free the pending array. + */ + struct rspamd_regexp_map_helper *re_map = entry->re_map; + const char *entry_name = entry->name; (void) error; if (exists) { - msg_debug_hyperscan("regexp map cache already exists for '%s', skipping compilation", entry->name); - rspamd_hs_helper_remap_send_notification(rmctx->ctx, rmctx->worker, entry->name); + msg_debug_hyperscan("regexp map cache already exists for '%s', skipping compilation", entry_name); + rspamd_hs_helper_remap_send_notification(rmctx->ctx, rmctx->worker, entry_name); rmctx->idx++; rspamd_hs_helper_compile_pending_regexp_maps_next(rmctx); + REF_RELEASE(rmctx); return; } @@ -1083,7 +1140,10 @@ rspamd_hs_helper_remap_exists_cb(gboolean success, rspamd_worker_set_busy(rmctx->worker, rmctx->ctx->event_loop, "compile regexp map"); /* Flush the busy notification before blocking on compilation */ ev_run(rmctx->ctx->event_loop, EVRUN_NOWAIT); - rspamd_regexp_map_compile_hs_to_cache_async(entry->re_map, rmctx->ctx->hs_dir, + /* Use saved re_map pointer - pending array may have been freed by ev_run */ + rmctx->compile_cb_called = FALSE; + REF_RETAIN(rmctx); + rspamd_regexp_map_compile_hs_to_cache_async(re_map, rmctx->ctx->hs_dir, rmctx->ctx->event_loop, rspamd_hs_helper_remap_compiled_cb, rmctx); } @@ -1107,6 +1167,7 @@ rspamd_hs_helper_compile_pending_regexp_maps_next(struct rspamd_hs_helper_remap_ char cache_key[rspamd_cryptobox_HASHBYTES * 2 + 1]; rspamd_snprintf(cache_key, sizeof(cache_key), "%*xs", (int) sizeof(entry->hash) / 2, entry->hash); + REF_RETAIN(rmctx); rspamd_hs_cache_lua_exists_async(cache_key, entry->name, rspamd_hs_helper_remap_exists_cb, rmctx); return; @@ -1140,8 +1201,7 @@ rspamd_hs_helper_compile_pending_regexp_maps_next(struct rspamd_hs_helper_remap_ } done: - rspamd_regexp_map_clear_pending(); - g_free(rmctx); + REF_RELEASE(rmctx); } static void @@ -1165,6 +1225,7 @@ rspamd_hs_helper_compile_pending_regexp_maps(struct hs_helper_ctx *ctx, rmctx->pending = pending; rmctx->count = count; rmctx->idx = 0; + REF_INIT_RETAIN(rmctx, rspamd_hs_helper_remap_async_ctx_dtor); rspamd_hs_helper_compile_pending_regexp_maps_next(rmctx); } diff --git a/src/libserver/maps/map_helpers.c b/src/libserver/maps/map_helpers.c index a1ee7fab11..627743f71e 100644 --- a/src/libserver/maps/map_helpers.c +++ b/src/libserver/maps/map_helpers.c @@ -2076,6 +2076,7 @@ struct rspamd_regexp_map_async_compile_ctx { char *cache_dir; unsigned char *serialized_db; gsize serialized_len; + gboolean callback_processed; }; static void @@ -2091,6 +2092,11 @@ rspamd_regexp_map_async_store_cb(gboolean success, (void) data; (void) len; + if (ctx->callback_processed) { + return; + } + ctx->callback_processed = TRUE; + if (!success) { g_set_error(&err, g_quark_from_static_string("regexp_map"), EINVAL, "failed to store regexp map to cache backend: %s", @@ -2274,6 +2280,7 @@ struct rspamd_regexp_map_async_load_ctx { void (*cb)(gboolean success, void *ud); void *ud; char *cache_dir; + gboolean callback_processed; }; static void @@ -2284,9 +2291,15 @@ rspamd_regexp_map_async_load_cb(gboolean success, void *ud) { struct rspamd_regexp_map_async_load_ctx *ctx = ud; - struct rspamd_map *map = ctx->re_map->map; + struct rspamd_map *map; gboolean result = FALSE; + if (ctx->callback_processed) { + return; + } + ctx->callback_processed = TRUE; + map = ctx->re_map->map; + if (!success || data == NULL || len == 0) { msg_warn_map("failed to load regexp map from cache backend: %s", error ? error : "no data"); diff --git a/src/libserver/re_cache.c b/src/libserver/re_cache.c index 6041724192..d4cc43bc26 100644 --- a/src/libserver/re_cache.c +++ b/src/libserver/re_cache.c @@ -2160,6 +2160,8 @@ struct rspamd_re_cache_hs_compile_cbdata { gboolean silent; unsigned int total; struct rspamd_worker *worker; + struct ev_loop *event_loop; + ev_timer *timer; void (*cb)(unsigned int ncompiled, GError *err, void *cbd); @@ -2168,6 +2170,7 @@ struct rspamd_re_cache_hs_compile_cbdata { /* Async state */ struct rspamd_re_class *current_class; enum rspamd_re_cache_compile_state state; + ref_entry_t ref; }; struct rspamd_re_cache_async_ctx { @@ -2175,20 +2178,46 @@ struct rspamd_re_cache_async_ctx { struct ev_loop *loop; ev_timer *w; int n; + gboolean callback_processed; }; static void rspamd_re_cache_compile_err(EV_P_ ev_timer *w, GError *err, struct rspamd_re_cache_hs_compile_cbdata *cbdata, bool is_fatal); +static void +rspamd_re_cache_hs_compile_cbdata_dtor(void *p) +{ + struct rspamd_re_cache_hs_compile_cbdata *cbdata = p; + + if (cbdata->timer && ev_is_active(cbdata->timer)) { + ev_timer_stop(cbdata->event_loop, cbdata->timer); + } + rspamd_heap_destroy(re_compile_queue, &cbdata->compile_queue); + g_free(cbdata->timer); + g_free(cbdata); +} + static void rspamd_re_cache_exists_cb(gboolean success, const unsigned char *data, gsize len, const char *err, void *ud) { struct rspamd_re_cache_async_ctx *ctx = ud; - struct rspamd_re_cache_hs_compile_cbdata *cbdata = ctx->cbdata; + struct rspamd_re_cache_hs_compile_cbdata *cbdata; const gboolean lua_backend = rspamd_hs_cache_has_lua_backend(); char path[PATH_MAX]; + if (ctx->callback_processed) { + return; + } + ctx->callback_processed = TRUE; + cbdata = ctx->cbdata; + + if (cbdata->worker && cbdata->worker->state != rspamd_worker_state_running) { + g_free(ctx); + REF_RELEASE(cbdata); + return; + } + if (success && len > 0) { /* Exists */ struct rspamd_re_class *re_class = cbdata->current_class; @@ -2241,20 +2270,33 @@ rspamd_re_cache_exists_cb(gboolean success, const unsigned char *data, gsize len cbdata->state = RSPAMD_RE_CACHE_COMPILE_STATE_COMPILING; } - ev_timer_again(ctx->loop, ctx->w); + ev_timer_again(cbdata->event_loop, cbdata->timer); g_free(ctx); + REF_RELEASE(cbdata); } static void rspamd_re_cache_save_cb(gboolean success, const unsigned char *data, gsize len, const char *err, void *ud) { struct rspamd_re_cache_async_ctx *ctx = ud; - struct rspamd_re_cache_hs_compile_cbdata *cbdata = ctx->cbdata; + struct rspamd_re_cache_hs_compile_cbdata *cbdata; + + if (ctx->callback_processed) { + return; + } + ctx->callback_processed = TRUE; + cbdata = ctx->cbdata; + + if (cbdata->worker && cbdata->worker->state != rspamd_worker_state_running) { + g_free(ctx); + REF_RELEASE(cbdata); + return; + } if (!success) { GError *gerr = g_error_new(rspamd_re_cache_quark(), EINVAL, "backend save failed: %s", err ? err : "unknown error"); - rspamd_re_cache_compile_err(ctx->loop, ctx->w, gerr, cbdata, false); + rspamd_re_cache_compile_err(cbdata->event_loop, cbdata->timer, gerr, cbdata, false); } else { struct rspamd_re_class *re_class = cbdata->current_class; @@ -2291,8 +2333,9 @@ rspamd_re_cache_save_cb(gboolean success, const unsigned char *data, gsize len, cbdata->state = RSPAMD_RE_CACHE_COMPILE_STATE_INIT; cbdata->current_class = NULL; - ev_timer_again(ctx->loop, ctx->w); + ev_timer_again(cbdata->event_loop, cbdata->timer); g_free(ctx); + REF_RELEASE(cbdata); } static void @@ -2301,13 +2344,10 @@ rspamd_re_cache_compile_err(EV_P_ ev_timer *w, GError *err, { if (is_fatal) { cbdata->cb(cbdata->total, err, cbdata->cbd); - ev_timer_stop(EV_A_ w); - g_free(w); - g_free(cbdata); + REF_RELEASE(cbdata); } else { msg_err("hyperscan compilation error: %s", err->message); - /* Continue compilation */ cbdata->state = RSPAMD_RE_CACHE_COMPILE_STATE_INIT; cbdata->current_class = NULL; ev_timer_again(EV_A_ w); @@ -2344,12 +2384,8 @@ rspamd_re_cache_compile_timer_cb(EV_P_ ev_timer *w, int revents) /* Stop if worker is terminating */ if (cbdata->worker && cbdata->worker->state != rspamd_worker_state_running) { - ev_timer_stop(EV_A_ w); cbdata->cb(cbdata->total, NULL, cbdata->cbd); - rspamd_heap_destroy(re_compile_queue, &cbdata->compile_queue); - g_free(w); - g_free(cbdata); - + REF_RELEASE(cbdata); return; } @@ -2362,12 +2398,8 @@ rspamd_re_cache_compile_timer_cb(EV_P_ ev_timer *w, int revents) rspamd_heap_pop(re_compile_queue, &cbdata->compile_queue); if (elt == NULL) { /* All done */ - ev_timer_stop(EV_A_ w); cbdata->cb(cbdata->total, NULL, cbdata->cbd); - rspamd_heap_destroy(re_compile_queue, &cbdata->compile_queue); - g_free(w); - g_free(cbdata); - + REF_RELEASE(cbdata); return; } @@ -2392,10 +2424,8 @@ rspamd_re_cache_compile_timer_cb(EV_P_ ev_timer *w, int revents) rspamd_snprintf(entity_name, sizeof(entity_name), "re_class:%s", rspamd_re_cache_type_to_string(re_class->type)); } + REF_RETAIN(cbdata); rspamd_hs_cache_lua_exists_async(re_class->hash, entity_name, rspamd_re_cache_exists_cb, ctx); - /* Don't stop timer here - the callback (rspamd_re_cache_exists_cb) handles - * restarting the timer. For file backend the callback runs synchronously - * within exists_async, so stopping here would undo the timer restart. */ return; } @@ -2637,14 +2667,12 @@ rspamd_re_cache_compile_timer_cb(EV_P_ ev_timer *w, int revents) rspamd_snprintf(entity_name, sizeof(entity_name), "re_class:%s", rspamd_re_cache_type_to_string(re_class->type)); } + REF_RETAIN(cbdata); rspamd_hs_cache_lua_save_async(re_class->hash, entity_name, combined, total_len, rspamd_re_cache_save_cb, ctx); g_free(combined); CLEANUP_ALLOCATED(false); g_free(hs_serialized); - /* Don't stop timer here - the callback (rspamd_re_cache_save_cb) handles - * restarting the timer. For file backend the callback runs synchronously - * within save_async, so stopping here would undo the timer restart. */ return; } else { @@ -2735,8 +2763,11 @@ int rspamd_re_cache_compile_hyperscan(struct rspamd_re_cache *cache, cbdata->silent = silent; cbdata->total = 0; cbdata->worker = worker; + cbdata->event_loop = event_loop; timer = g_malloc0(sizeof(*timer)); timer->data = (void *) cbdata; + cbdata->timer = timer; + REF_INIT_RETAIN(cbdata, rspamd_re_cache_hs_compile_cbdata_dtor); ev_timer_init(timer, rspamd_re_cache_compile_timer_cb, timer_interval, timer_interval); diff --git a/src/libutil/multipattern.c b/src/libutil/multipattern.c index cb0e9bb9ad..b681c9598b 100644 --- a/src/libutil/multipattern.c +++ b/src/libutil/multipattern.c @@ -1489,6 +1489,7 @@ struct rspamd_multipattern_hs_cache_async_ctx { char *cache_key; rspamd_multipattern_hs_cache_cb_t cb; void *ud; + gboolean callback_processed; }; static void @@ -1512,6 +1513,11 @@ rspamd_multipattern_hs_cache_save_cb(gboolean success, (void) data; (void) len; + if (ctx->callback_processed) { + return; + } + ctx->callback_processed = TRUE; + if (success) { msg_info("saved hyperscan multipattern cache %s to Lua backend", ctx->cache_key ? ctx->cache_key : "(null)");