From: Christopher Faulet Date: Thu, 21 Apr 2022 09:52:07 +0000 (+0200) Subject: BUG/MEDIUM: conn-stream: Set back CS to RDY state when the appctx is created X-Git-Tag: v2.6-dev7~14 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a33ff7a8a763f1e92a54f16119c7430a76367350;p=thirdparty%2Fhaproxy.git BUG/MEDIUM: conn-stream: Set back CS to RDY state when the appctx is created When an appctx is created on the server side, we now set the corresponding conn-stream to ready state (CS_ST_RDY). When it happens, the backend conn-stream is in CS_ST_INI state. It is not consistant to let the conn-stream in this state because it means it is possible to have a target installed in CS_ST_INI state, while with a connection, the conn-stream is switch to CS_ST_RDY or CS_ST_EST state. It is especially anbiguous because we may be tempted to think there is no endpoint attached to the conn-stream before the CS_ST_CON state. And it is indeed the reason for a bug leading to a crash because a cs_detach_endp() is performed if an abort is detected on the backend conn-stream in CS_ST_INI state. With a mux or a appctx attached to the conn-stream, "->endp" field is set to NULL. It is unexpected. The API will be changed to be sure it is not possible. But it exposes a consistency issue with applets. So, the conn-stream must not stay in CS_ST_INI state when an appctx is attached. But there is no reason to set it in CS_ST_REQ. The conn-stream must be set to CS_ST_RDY to handle applets and connections in the same way. Note that if only the target is set but no appctx is created, the backend conn-stream is switched from CS_ST_INI to CS_ST_REQ state to be able to create the corresponding appctx. This part is unchanged. This patch depends on the commit "MINOR: backend: Don't allow to change backend applet". The ambiguity exists on previous versions. But the issue is 2.6-specific. Thus, no backport is needed. --- diff --git a/src/backend.c b/src/backend.c index 12f9a651b6..6bff6ffe5d 100644 --- a/src/backend.c +++ b/src/backend.c @@ -2113,7 +2113,7 @@ abort_connection: /* This function initiates a server connection request on a conn-stream * already in CS_ST_REQ state. Upon success, the state goes to CS_ST_ASS for * a real connection to a server, indicating that a server has been assigned, - * or CS_ST_EST for a successful connection to an applet. It may also return + * or CS_ST_RDY for a successful connection to an applet. It may also return * CS_ST_QUE, or CS_ST_CLO upon error. */ void back_handle_st_req(struct stream *s) @@ -2126,12 +2126,14 @@ void back_handle_st_req(struct stream *s) DBG_TRACE_ENTER(STRM_EV_STRM_PROC|STRM_EV_CS_ST, s); if (unlikely(obj_type(s->target) == OBJ_TYPE_APPLET)) { - /* the applet directly goes to the EST state */ - struct appctx *appctx = cs_appctx(s->csb); - - if (!appctx) - appctx = cs_applet_create(cs, objt_applet(s->target)); + struct appctx *appctx; + /* The target is an applet but the CS is in CS_ST_REQ. Thus it + * means no appctx are attached to the CS. Otherwise, it will be + * in CS_ST_RDY state. So, try to create the appctx now. + */ + BUG_ON(cs_appctx(cs)); + appctx = cs_applet_create(cs, objt_applet(s->target)); if (!appctx) { /* No more memory, let's immediately abort. Force the * error code to ignore the ERR_LOCAL which is not a @@ -2150,15 +2152,7 @@ void back_handle_st_req(struct stream *s) goto end; } - if (tv_iszero(&s->logs.tv_request)) - s->logs.tv_request = now; - s->logs.t_queue = tv_ms_elapsed(&s->logs.tv_accept, &now); - cs->state = CS_ST_EST; - s->conn_err_type = STRM_ET_NONE; - be_set_sess_last(s->be); - DBG_TRACE_STATE("applet registered", STRM_EV_STRM_PROC|STRM_EV_CS_ST, s); - /* let back_establish() finish the job */ goto end; } @@ -2406,6 +2400,19 @@ void back_handle_st_rdy(struct stream *s) struct channel *rep = &s->res; DBG_TRACE_ENTER(STRM_EV_STRM_PROC|STRM_EV_CS_ST, s); + + if (unlikely(obj_type(s->target) == OBJ_TYPE_APPLET)) { + /* Here the appctx must exists because the CS was set to + * CS_ST_RDY state when the appctx was created. + */ + BUG_ON(!cs_appctx(s->csb)); + + if (tv_iszero(&s->logs.tv_request)) + s->logs.tv_request = now; + s->logs.t_queue = tv_ms_elapsed(&s->logs.tv_accept, &now); + be_set_sess_last(s->be); + } + /* We know the connection at least succeeded, though it could have * since met an error for any other reason. At least it didn't time out * even though the timeout might have been reported right after success. diff --git a/src/conn_stream.c b/src/conn_stream.c index 29bf49b050..e753749ca9 100644 --- a/src/conn_stream.c +++ b/src/conn_stream.c @@ -468,6 +468,8 @@ struct appctx *cs_applet_create(struct conn_stream *cs, struct applet *app) appctx->t->nice = __cs_strm(cs)->task->nice; cs_cant_get(cs); appctx_wakeup(appctx); + + cs->state = CS_ST_RDY; return appctx; }