[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 105b6a0f87620e083793e7532abccee13cf6aa9e..f6923a0285fc123f493b565ba30fd42c2cfaae59 100644 (file)
--- 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 a1ee7fab11d21d46f3b8370cc6cb55af878a410a..627743f71e104d7144ba5d3d181753ab4ebc23dc 100644 (file)
--- 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 604172419203fe45a0f8f1ff810eaadb8ebef8db..d4cc43bc26dd5d0310bd93e79830f851251bddd6 100644 (file)
--- 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 cb0e9bb9add3b589eea54e24690e56f476f7dca6..b681c9598bb0d2d641d94967d016efaace36794f 100644 (file)
--- 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)");