From 36b536d6c8f94e66109f11973e0c975420b98369 Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Wed, 20 Nov 2019 11:56:33 +0100 Subject: [PATCH] BUG/MEDIUM: stream-int: Don't loose events on the CS when an EOS is reported In si_cs_recv(), when a shutdown for reads is handled, the conn-stream may be closed. It happens when the ouput channel is closed for writes or if SI_FL_NOHALF is set on the stream-interface. In this case, conn-stream's flags are reset. Thus, if an error (CS_FL_ERROR) or an end of input (CS_FL_EOI) is reported by the mux, the event is lost. si_cs_recv() does not report these events by itself. It relies on si_cs_process() to report them to the stream-interface and/or the channel. For instance, if CS_FL_EOS and CS_FL_EOI are set by the H1 multiplexer during a call to si_cs_recv() on the server side, if the conn-stream is closed (read0 + SI_FL_NOHALF), the CS_FL_EOI flag is lost. Thus, this may lead the stream to interpret it as a server abort. Now, conn-stream's flags are processed at the end of si_cs_recv(). The function is responsible to set the right flags on the stream-interface and/or the channel. Due to this patch, the function is now almost linear. Except some early checks at the beginning, there is only one return statement. It also fixes a potential bug because of an inconsistency between the splicing and the buffered receipt. On the first case, CS_FL_EOS if handled before errors on the connection or the conn-stream. On the second one, it is the opposite. This patch must be backported to 2.0 and 1.9. --- src/stream_interface.c | 62 ++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/stream_interface.c b/src/stream_interface.c index f1513a81f5..9503d12f65 100644 --- a/src/stream_interface.c +++ b/src/stream_interface.c @@ -594,6 +594,10 @@ static int si_cs_process(struct conn_stream *cs) * after process_stream() noticed there were an error, and decided * to retry to connect, the connection may still have CO_FL_ERROR, * and we don't want to add SI_FL_ERR back + * + * Note: This test is only required because si_cs_process is also the SI + * wake callback. Otherwise si_cs_recv()/si_cs_send() already take + * care of it. */ if (si->state >= SI_ST_CON && (conn->flags & CO_FL_ERROR || cs->flags & CS_FL_ERROR)) @@ -619,7 +623,12 @@ static int si_cs_process(struct conn_stream *cs) } /* Report EOI on the channel if it was reached from the mux point of - * view. */ + * view. + * + * Note: This test is only required because si_cs_process is also the SI + * wake callback. Otherwise si_cs_recv()/si_cs_send() already take + * care of it. + */ if ((cs->flags & CS_FL_EOI) && !(ic->flags & CF_EOI)) ic->flags |= (CF_EOI|CF_READ_PARTIAL); @@ -1226,7 +1235,7 @@ int si_cs_recv(struct conn_stream *cs) /* stop here if we reached the end of data */ if (cs->flags & CS_FL_EOS) - goto out_shutdown_r; + goto end_recv; /* stop immediately on errors. Note that we DON'T want to stop on * POLL_ERR, as the poller might report a write error while there @@ -1238,7 +1247,7 @@ int si_cs_recv(struct conn_stream *cs) if (!conn_xprt_ready(conn)) return 0; if (conn->flags & CO_FL_ERROR || cs->flags & CS_FL_ERROR) - return 1; // We want to make sure si_cs_wake() is called, so that process_strema is woken up, on failure + goto end_recv; } /* prepare to detect if the mux needs more room */ @@ -1294,11 +1303,8 @@ int si_cs_recv(struct conn_stream *cs) ic->flags |= CF_READ_PARTIAL; } - if (cs->flags & CS_FL_EOS) - goto out_shutdown_r; - - if (conn->flags & CO_FL_ERROR || cs->flags & CS_FL_ERROR) - return 1; + if (conn->flags & CO_FL_ERROR || cs->flags & (CS_FL_EOS|CS_FL_ERROR)) + goto end_recv; if (conn->flags & CO_FL_WAIT_ROOM) { /* the pipe is full or we have read enough data that it @@ -1468,32 +1474,36 @@ int si_cs_recv(struct conn_stream *cs) } end_recv: - if (conn->flags & CO_FL_ERROR || cs->flags & CS_FL_ERROR) - return 1; + /* Report EOI on the channel if it was reached from the mux point of + * view. */ + if ((cs->flags & CS_FL_EOI) && !(ic->flags & CF_EOI)) + ic->flags |= (CF_EOI|CF_READ_PARTIAL); - if (cs->flags & CS_FL_EOS) + if (conn->flags & CO_FL_ERROR || cs->flags & CS_FL_ERROR) { + cs->flags |= CS_FL_ERROR; + si->flags |= SI_FL_ERR; + } + else if (cs->flags & CS_FL_EOS) { /* connection closed */ - goto out_shutdown_r; - - /* Subscribe to receive events if we're blocking on I/O */ - if (!si_rx_blocked(si)) { + if (conn->flags & CO_FL_CONNECTED) { + /* we received a shutdown */ + ic->flags |= CF_READ_NULL; + if (ic->flags & CF_AUTO_CLOSE) + channel_shutw_now(ic); + stream_int_read0(si); + } + } + else if (!si_rx_blocked(si)) { + /* Subscribe to receive events if we're blocking on I/O */ conn->mux->subscribe(cs, SUB_RETRY_RECV, &si->wait_event); si_rx_endp_done(si); } else { si_rx_endp_more(si); } - return (cur_read != 0) || si_rx_blocked(si) || (cs->flags & CS_FL_EOI); - - out_shutdown_r: - if (conn->flags & CO_FL_CONNECTED) { - /* we received a shutdown */ - ic->flags |= CF_READ_NULL; - if (ic->flags & CF_AUTO_CLOSE) - channel_shutw_now(ic); - stream_int_read0(si); - } - return 1; + return (cur_read != 0) || + si_rx_blocked(si) || + (cs->flags & (CS_FL_EOI|CS_FL_EOS|CS_FL_ERROR)); } /* -- 2.39.5