BUG/MEDIUM: spoe: Let an agent outlive its applets during a backend deletion

The deletion of a backend holding a SPOE filter destroys the SPOE agent with
the filter deinitialization. But the agent owns an internal frontend proxy,
with its streams and the SPOE applets processing its frames, which are not
attached to the filtered backend: they may outlive the last stream of this
backend, so the agent may be freed while its applets, and the streams of its
applets, still reference it. This is a use-after-free, reachable with the
"del backend" CLI command: no server is required on the filtered backend and
the SPOE streams are attached to the internal frontend of the agent, so an
unpublished backend without attached stream can be deleted while its agent
is still processing frames.

Add a reference count on the SPOE agent: the filter configuration owns one
reference, dropped during its deinitialization, and each applet holds one,
taken when it is created and dropped when its applet context is destroyed,
with the new applet destroy callback. The agent is released when its last
reference is dropped. A backend holding a SPOE filter can now be deleted at
any time: its agents stay alive as long as their filter configuration and
their applets reference them.

The applet reference cannot be dropped with the applet release callback: it
may run during the processing of the applet stream, which may then be
requeued for a last run, for instance while the connection to the agent is
being closed, and dereference the agent internal frontend again, for the log
message of its close for instance. The applet destroy callback is used
instead: it runs during the teardown of the applet stream, from sc_destroy()
inside stream_free(). So the applet stream is guaranteed to never run again.

The internal frontend of the agent, previously embedded in the agent
structure and initialized with setup_new_proxy(), is now allocated with
alloc_new_proxy(), like any other internal proxy, and released with
proxy_drop(), when the agent is released. It may then survive its agent,
which is released during the teardown of the streams of its last applets:
the streams attached to the frontend hold a reference on it during their own
teardown. So the frontend is released after the teardown of its last stream
completed.

This will also be needed for the dynamically created backends, which will be
able to inherit an active SPOE filter instance from their defaults section
and are meant to be deleted at any time.

This patch relies on:

  * MINOR: applet: Add a destroy callback to the applets
  * MEDIUM: stream: Hold a reference on the frontend during stream_free()

This patch should be backported as far as 3.3 with commits above but only
after a period of observation.

diff --git a/include/haproxy/spoe-t.h b/include/haproxy/spoe-t.h
index de762fcdcf77fa10c8b0de096ff9b22aa3aad501..4333afb8ea9d83f7afd5bb1ce821c8a3a621caed 100644 (file)
--- a/include/haproxy/spoe-t.h
+++ b/include/haproxy/spoe-t.h
@@ -145,7 +145,7 @@ struct spoe_agent {
                 char         *file;           /* file where the SPOE agent appears */
                 int           line;           /* line where the SPOE agent appears */
         } conf;                               /* config information */
-       struct proxy       fe;                /* Agent frontend */
+       struct proxy       *fe;               /* Agent frontend */
        union {
                struct proxy *be;             /* Backend used by this agent */
                char         *name;           /* Backend name used during conf parsing */
@@ -171,6 +171,7 @@ struct spoe_agent {
        struct list messages;                 /* list of all messages attached to this SPOE agent */
 
        char *engine_id;                      /* engine-id string */
+        int refcount;                         /* # of references on this agent */
 
        struct {
                unsigned long long nb_processed; /* # of frames processed by the SPOE */

diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index 0282677657bc070596f31a758867452598a6d937..d9fe38635bfe94921f59ee8f1fbaefd4f13c57a0 100644 (file)
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -323,11 +323,34 @@ static void spoe_release_agent(struct spoe_agent *agent)
        }
        free(agent->events);
        free(agent->engine_id);
-       if (agent->fe.id)
-               deinit_proxy(&agent->fe);
+
+       proxy_drop(agent->fe);
        free(agent);
 }
 
+/* Takes a reference on the SPOE agent <agent>.
+ */
+static inline void spoe_agent_take(struct spoe_agent *agent)
+{
+       _HA_ATOMIC_INC(&agent->refcount);
+}
+
+/* Drops a reference on the SPOE agent <agent>, releasing it when it was the
+ * last one. The agent is owned by its filter configuration, which drops its
+ * reference during its deinitialization, and its applets hold one reference
+ * each, dropped when their applet context is destroyed. So the agent is
+ * released once its filter configuration is destroyed and the applet contexts
+ * of all its applets were destroyed, hence after the teardown of their streams
+ * completed.
+ */
+static void spoe_agent_drop(struct spoe_agent *agent)
+{
+       if (_HA_ATOMIC_SUB_FETCH(&agent->refcount, 1) != 0)
+               return;
+
+       spoe_release_agent(agent);
+}
+
 static const char *spoe_event_str[SPOE_EV_EVENTS] = {
        [SPOE_EV_ON_CLIENT_SESS] = "on-client-session",
        [SPOE_EV_ON_TCP_REQ_FE] = "on-frontend-tcp-request",
@@ -384,7 +407,7 @@ static int spoe_init_appctx(struct appctx *appctx)
        struct spoe_agent *agent = spoe_appctx->agent;
        struct stream *s;
 
-       if (appctx_finalize_startup(appctx, &agent->fe, &spoe_appctx->spoe_ctx->buffer) == -1)
+       if (appctx_finalize_startup(appctx, agent->fe, &spoe_appctx->spoe_ctx->buffer) == -1)
                goto error;
 
        spoe_appctx->owner = appctx;
@@ -431,7 +454,10 @@ static void spoe_release_appctx(struct appctx *appctx)
        if (spoe_appctx == NULL)
                return;
 
-       appctx->svcctx = NULL;
+       /* the SPOE applet context is kept in svcctx until the applet context is
+        * destroyed (see spoe_destroy_appctx()), to drop the reference on the
+        * agent after the teardown of the applet stream completed.
+        */
        appctx_strm(appctx)->parent = NULL;
 
        /* Shutdown the server connection, if needed */
@@ -451,9 +477,23 @@ static void spoe_release_appctx(struct appctx *appctx)
                }
                task_wakeup(spoe_appctx->spoe_ctx->strm->task, TASK_WOKEN_MSG);
        }
+}
 
-  end:
-       /* Release allocated memory */
+/* Callback function called when a SPOE applet context is destroyed, from
+ * __appctx_free(). This is the last user of the applet context: it happens
+ * during the teardown of the applet stream (from sc_destroy(), inside
+ * stream_free()). So the stream cannot reference the agent internal frontend
+ * anymore. Drop the applet reference on the agent, taken when the applet was
+ * created, and release the SPOE applet context.
+ */
+static void spoe_destroy_appctx(struct appctx *appctx)
+{
+       struct spoe_appctx *spoe_appctx = SPOE_APPCTX(appctx);
+
+       if (!spoe_appctx)
+               return;
+
+       spoe_agent_drop(spoe_appctx->agent);
        pool_free(pool_head_spoe_appctx, spoe_appctx);
 }
 
@@ -557,6 +597,7 @@ struct applet spoe_applet = {
        .rcv_buf = appctx_raw_rcv_buf,
        .snd_buf = appctx_raw_snd_buf,
        .release = spoe_release_appctx,
+       .destroy = spoe_destroy_appctx,
 };
 
 /* Create a SPOE applet. On success, the created applet is returned, else
@@ -578,6 +619,8 @@ static struct appctx *spoe_create_appctx(struct spoe_context *ctx)
        spoe_appctx->spoe_ctx        = ctx;
        ctx->spoe_appctx             = spoe_appctx;
 
+       spoe_agent_take(agent);
+
        if ((appctx = appctx_new_here(&spoe_applet, NULL)) == NULL)
                goto out_free_spoe_appctx;
 
@@ -591,16 +634,17 @@ static struct appctx *spoe_create_appctx(struct spoe_context *ctx)
        /* Error unrolling */
  out_free_appctx:
        appctx_free_on_early_error(appctx);
+       if (ctx->spoe_appctx == spoe_appctx)
+               ctx->spoe_appctx = NULL;
+       goto out_error;
+
  out_free_spoe_appctx:
-       /* the context must not be left pointing to the applet we're about to
-        * release, the caller still uses it on the error path.
-        */
        if (ctx->spoe_appctx == spoe_appctx)
                ctx->spoe_appctx = NULL;
        pool_free(pool_head_spoe_appctx, spoe_appctx);
+       spoe_agent_drop(agent);
  out_error:
-       send_log(&agent->fe, LOG_EMERG, "SPOE: [%s] failed to create SPOE applet\n", agent->id);
- out:
+       send_log(agent->fe, LOG_EMERG, "SPOE: [%s] failed to create SPOE applet\n", agent->id);
 
        return NULL;
 }
@@ -1095,8 +1139,8 @@ static int spoe_process_group(struct stream *s, struct spoe_context *ctx,
 
        ret = spoe_process_messages(s, ctx, &group->messages, dir, SPOE_MSGS_BY_GROUP);
        if (ret && ctx->stats.t_process != -1) {
-               if (ctx->status_code || !(agent->fe.options2 & PR_O2_NOLOGNORM))
-                       send_log(&agent->fe, (!ctx->status_code ? LOG_NOTICE : LOG_WARNING),
+               if (ctx->status_code || !(agent->fe->options2 & PR_O2_NOLOGNORM))
+                       send_log(agent->fe, (!ctx->status_code ? LOG_NOTICE : LOG_WARNING),
                                 "SPOE: [%s] <GROUP:%s> sid=%u st=%u %ld %llu/%llu\n",
                                 agent->id, group->id, s->uniq_id, ctx->status_code, ctx->stats.t_process,
                                 agent->counters.nb_errors, agent->counters.nb_processed);
@@ -1120,8 +1164,8 @@ static int spoe_process_event(struct stream *s, struct spoe_context *ctx,
 
        ret = spoe_process_messages(s, ctx, &(ctx->events[ev]), dir, SPOE_MSGS_BY_EVENT);
        if (ret && ctx->stats.t_process != -1) {
-               if (ctx->status_code || !(agent->fe.options2 & PR_O2_NOLOGNORM))
-                       send_log(&agent->fe, (!ctx->status_code ? LOG_NOTICE : LOG_WARNING),
+               if (ctx->status_code || !(agent->fe->options2 & PR_O2_NOLOGNORM))
+                       send_log(agent->fe, (!ctx->status_code ? LOG_NOTICE : LOG_WARNING),
                                 "SPOE: [%s] <EVENT:%s> sid=%u st=%u %ld %llu/%llu\n",
                                 agent->id, spoe_event_str[ev], s->uniq_id, ctx->status_code, ctx->stats.t_process,
                                 agent->counters.nb_errors, agent->counters.nb_processed);
@@ -1246,16 +1290,16 @@ static int spoe_init(struct proxy *px, struct flt_conf *fconf)
 
        /* conf->agent->fe was already initialized during the config
         * parsing. Finish initialization. */
-       conf->agent->fe.mode = PR_MODE_SPOP;
-       conf->agent->fe.maxconn = 0;
-       conf->agent->fe.options2 |= PR_O2_INDEPSTR;
-       conf->agent->fe.conn_retries = CONN_RETRIES;
-       conf->agent->fe.accept = frontend_accept;
-       LIST_INIT(&conf->agent->fe.servers);
-       conf->agent->fe.timeout.client = TICK_ETERNITY;
-       conf->agent->fe.fe_req_ana = AN_REQ_SWITCHING_RULES;
+       conf->agent->fe->mode = PR_MODE_SPOP;
+       conf->agent->fe->maxconn = 0;
+       conf->agent->fe->options2 |= PR_O2_INDEPSTR;
+       conf->agent->fe->conn_retries = CONN_RETRIES;
+       conf->agent->fe->accept = frontend_accept;
+       LIST_INIT(&conf->agent->fe->servers);
+       conf->agent->fe->timeout.client = TICK_ETERNITY;
+       conf->agent->fe->fe_req_ana = AN_REQ_SWITCHING_RULES;
 
-       proxy_init_per_thr(&conf->agent->fe);
+       proxy_init_per_thr(conf->agent->fe);
 
        conf->agent->engine_id = generate_pseudo_uuid();
        if (conf->agent->engine_id == NULL)
@@ -1273,7 +1317,7 @@ static void spoe_deinit(struct proxy *px, struct flt_conf *fconf)
        if (conf) {
                struct spoe_agent *agent = conf->agent;
 
-               spoe_release_agent(agent);
+               spoe_agent_drop(agent);
                free(conf->id);
                free(conf);
        }
@@ -1345,7 +1389,7 @@ static int spoe_check(struct proxy *px, struct flt_conf *fconf)
                return 1;
        }
 
-       if (postresolve_logger_list(NULL, &conf->agent->fe.loggers, "SPOE agent", conf->agent->id) & ERR_CODE)
+       if (postresolve_logger_list(NULL, &conf->agent->fe->loggers, "SPOE agent", conf->agent->id) & ERR_CODE)
                return 1;
 
        ha_free(&conf->agent->b.name);
@@ -1365,7 +1409,7 @@ static int spoe_start(struct stream *s, struct filter *filter)
        struct spoe_context *ctx;
 
        if ((ctx = spoe_create_context(s, filter)) == NULL) {
-               send_log(&agent->fe, LOG_EMERG,
+               send_log(agent->fe, LOG_EMERG,
                         "SPOE: [%s] failed to create SPOE context\n",
                         agent->id);
                return 0;
@@ -1591,6 +1635,11 @@ static int cfg_parse_spoe_agent(const char *file, int linenum, char **args, int
                curagent->flags          = SPOE_FL_PIPELINING;
                curagent->max_frame_size = SPOP_MAX_FRAME_SIZE;
 
+               /* The agent is owned by the filter configuration, which takes its
+                * reference here. The applets will take theirs when created.
+                */
+               curagent->refcount       = 1;
+
                if ((curagent->events = calloc(SPOE_EV_EVENTS, sizeof(*curagent->events))) == NULL) {
                        ha_alert("parsing [%s:%d] : out of memory.\n", file, linenum);
                        err_code |= ERR_ALERT | ERR_ABORT;
@@ -2610,19 +2659,19 @@ static int parse_spoe_flt(char **args, int *cur_arg, struct proxy *px,
 
        /* Start agent's proxy initialization here. It will be finished during
         * the filter init. */
-        memset(&conf->agent->fe, 0, sizeof(conf->agent->fe));
-       if (!setup_new_proxy(&conf->agent->fe, conf->agent->id, PR_CAP_FE | PR_CAP_INT, err)) {
+       conf->agent->fe = alloc_new_proxy(conf->agent->id, PR_CAP_FE | PR_CAP_INT, err);
+       if (!conf->agent->fe) {
                memprintf(err, "SPOE agent '%s': %s",
                          curagent->id, *err);
                goto error;
        }
-       conf->agent->fe.parent    = conf->agent;
-       conf->agent->fe.options  |= curpxopts;
-       conf->agent->fe.options2 |= curpxopts2;
+       conf->agent->fe->parent    = conf->agent;
+       conf->agent->fe->options  |= curpxopts;
+       conf->agent->fe->options2 |= curpxopts2;
 
        list_for_each_entry_safe(logger, loggerback, &curloggers, list) {
                LIST_DELETE(&logger->list);
-               LIST_APPEND(&conf->agent->fe.loggers, &logger->list);
+               LIST_APPEND(&conf->agent->fe->loggers, &logger->list);
        }
 
        list_for_each_entry_safe(ph, phback, &curmphs, list) {