From 53c3046898633e56f74f7f05fb38cabeea1c87a1 Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Mon, 28 Apr 2025 08:08:06 +0200 Subject: [PATCH] BUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT frame In the SPOE specification, when an error occurred on the SPOP connection, HAProxy must send a DISCONNECT frame and wait for the agent DISCONNECT frame in return before trully closing the connection. However, this part was not properly handled by the SPOP multiplexer. In this case, the SPOP connection should be in the CLOSING state. But this state was not used at all. Depending on when the error was encountered, the connection could be closed immediately, without sending any DISCONNECT frame. It was the case when an early error was detected during the AGENT-HELLO frame parsing. Or it could be moved from ERROR to FRAME_H state, as if no error were detected. This case was less dramatic than it seemed because some flags were also set to prevent any problem. But it was not obvious. So now, the SPOP connection is properly switch to CLOSING state when an DISCONNECT is sent to the agent to be able to wait for its DISCONNECT in reply. spop_process_demux() was updated to parse frames in that state and some validity checks was added. This patch must be backport to 3.1. --- src/mux_spop.c | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/mux_spop.c b/src/mux_spop.c index 1dc4b3588..953692756 100644 --- a/src/mux_spop.c +++ b/src/mux_spop.c @@ -605,7 +605,7 @@ static int spop_avail_streams(struct connection *conn) int ret1, ret2; /* Don't open new stream if the connection is closed */ - if (spop_conn->state == SPOP_CS_CLOSED) + if (spop_conn->state >= SPOP_CS_ERROR || (spop_conn->flags & (SPOP_CF_ERR_PENDING|SPOP_CF_ERROR))) return 0; /* May be negative if this setting has changed */ @@ -1291,7 +1291,8 @@ static void spop_strm_wake_one_stream(struct spop_strm *spop_strm) spop_strm_close(spop_strm); } - if (spop_conn->state == SPOP_CS_CLOSED || (spop_conn->flags & (SPOP_CF_ERR_PENDING|SPOP_CF_ERROR))) { + + if (spop_conn->state >= SPOP_CS_ERROR || (spop_conn->flags & (SPOP_CF_ERR_PENDING|SPOP_CF_ERROR))) { se_fl_set_error(spop_strm->sd); spop_strm_propagate_term_flags(spop_conn, spop_strm); if (!spop_strm->sd->abort_info.info) { @@ -1555,6 +1556,7 @@ static int spop_conn_send_disconnect(struct spop_conn *spop_conn) spop_set_frame_size(outbuf.area, outbuf.data - 4); b_add(mbuf, outbuf.data); spop_conn->flags |= SPOP_CF_DISCO_SENT; + spop_conn->state = SPOP_CS_CLOSING; ret = 1; switch (spop_conn->errcode) { @@ -1782,12 +1784,11 @@ static int spop_conn_handle_hello(struct spop_conn *spop_conn) TRACE_PROTO("SPOP AGENT HELLO frame rcvd", SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO, spop_conn->conn, 0, 0, (size_t[]){spop_conn->dfl}); b_del(&spop_conn->dbuf, spop_conn->dfl); spop_conn->dfl = 0; + spop_conn->state = SPOP_CS_FRAME_H; spop_wake_unassigned_streams(spop_conn); TRACE_LEAVE(SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO, spop_conn->conn); return 1; fail: - spop_conn->state = SPOP_CS_CLOSED; - TRACE_STATE("switching to CLOSED", SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO, spop_conn->conn); TRACE_DEVEL("leaving on error", SPOP_EV_RX_FRAME|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn); return 0; } @@ -1973,7 +1974,6 @@ static int spop_conn_handle_ack(struct spop_conn *spop_conn, struct spop_strm *s sent = b_xfer(rxbuf, dbuf, flen); BUG_ON(sent != flen); - /* b_del(&spop_conn->dbuf, sent); */ spop_conn->dfl -= sent; end: @@ -2062,27 +2062,35 @@ static void spop_process_demux(struct spop_conn *spop_conn) TRACE_ENTER(SPOP_EV_SPOP_CONN_WAKE, spop_conn->conn); - if (spop_conn->state >= SPOP_CS_ERROR) + if (spop_conn->state == SPOP_CS_CLOSED) goto out; - if (unlikely(spop_conn->state < SPOP_CS_FRAME_H)) { + if (unlikely(spop_conn->state < SPOP_CS_FRAME_H || spop_conn->state == SPOP_CS_CLOSING)) { if (spop_conn->state == SPOP_CS_HA_HELLO) { TRACE_STATE("waiting AGENT HELLO frame to be sent", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO, spop_conn->conn); goto out; } - if (spop_conn->state == SPOP_CS_AGENT_HELLO) { - /* ensure that what is pending is a valid AGENT HELLO frame. */ - TRACE_STATE("receiving AGENT HELLO frame header", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn); + else { /* AGENT_HELLO OR CLOSING */ + /* ensure that what is pending is a valid AGENT HELLO/DISCONNECT frame. */ + TRACE_STATE("receiving AGENT HELLO/DISCONNECT frame header", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn); if (!spop_get_frame_hdr(&spop_conn->dbuf, &hdr)) { spop_conn->flags |= SPOP_CF_DEM_SHORT_READ; TRACE_ERROR("header frame not available yet", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn); goto done; } - if (hdr.sid || hdr.fid || hdr.type != SPOP_FRM_T_AGENT_HELLO || !(hdr.flags & SPOP_FRM_FL_FIN)) { + if (spop_conn->state == SPOP_CS_AGENT_HELLO && hdr.type != SPOP_FRM_T_AGENT_HELLO) { spop_conn_error(spop_conn, SPOP_ERR_INVALID); spop_conn->state = SPOP_CS_CLOSED; - TRACE_ERROR("unexpected frame type or flags", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn); + TRACE_ERROR("unexpected frame type (AGENT HELLO expected)", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn); + TRACE_STATE("switching to CLOSED", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn); + goto done; + } + + if ((hdr.type == SPOP_FRM_T_AGENT_HELLO || hdr.type == SPOP_FRM_T_AGENT_DISCON) && (hdr.sid || hdr.fid || !(hdr.flags & SPOP_FRM_FL_FIN))) { + spop_conn_error(spop_conn, SPOP_ERR_INVALID); + spop_conn->state = SPOP_CS_CLOSED; + TRACE_ERROR("unexpected frame meta-data", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn); TRACE_STATE("switching to CLOSED", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR|SPOP_EV_RX_HELLO|SPOP_EV_SPOP_CONN_ERR, spop_conn->conn); goto done; } @@ -2128,13 +2136,14 @@ static void spop_process_demux(struct spop_conn *spop_conn) break; } + spop_conn->state = SPOP_CS_FRAME_P; + new_frame: spop_conn->dsi = hdr.sid; spop_conn->dfi = hdr.fid; spop_conn->dft = hdr.type; spop_conn->dfl = hdr.len; spop_conn->dff = hdr.flags; - spop_conn->state = SPOP_CS_FRAME_P; TRACE_STATE("SPOP frame header rcvd, switching to FRAME_P", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn); /* Perform sanity check on the frame header */ @@ -2212,12 +2221,10 @@ static void spop_process_demux(struct spop_conn *spop_conn) switch (spop_conn->dft) { case SPOP_FRM_T_AGENT_HELLO: - if (spop_conn->state == SPOP_CS_FRAME_P) - ret = spop_conn_handle_hello(spop_conn); + ret = spop_conn_handle_hello(spop_conn); break; case SPOP_FRM_T_AGENT_DISCON: - if (spop_conn->state == SPOP_CS_FRAME_P) - ret = spop_conn_handle_disconnect(spop_conn); + ret = spop_conn_handle_disconnect(spop_conn); break; case SPOP_FRM_T_AGENT_ACK: if (spop_conn->state == SPOP_CS_FRAME_P) @@ -2252,7 +2259,7 @@ static void spop_process_demux(struct spop_conn *spop_conn) b_del(&spop_conn->dbuf, ret); spop_conn->dfl -= ret; } - if (!spop_conn->dfl) { + if (!spop_conn->dfl && spop_conn->state != SPOP_CS_FRAME_P) { TRACE_STATE("switching to FRAME_H", SPOP_EV_RX_FRAME|SPOP_EV_RX_FHDR, spop_conn->conn); spop_conn->state = SPOP_CS_FRAME_H; } @@ -2590,7 +2597,7 @@ static int spop_process(struct spop_conn *spop_conn) (b_data(&spop_conn->dbuf) || (spop_conn->flags & SPOP_CF_RCVD_SHUT))) { spop_process_demux(spop_conn); - if (spop_conn->state >= SPOP_CS_ERROR || (spop_conn->flags & SPOP_CF_ERROR)) + if (spop_conn->state == SPOP_CS_CLOSED || (spop_conn->flags & SPOP_CF_ERROR)) b_reset(&spop_conn->dbuf); if (b_room(&spop_conn->dbuf))