BUG/MAJOR: htx: Don't swap buffers for empty HTX message with an error

Recent fix of some HTX muxes to drain remaining data when the stream is in
closed state revealed a bug, mainly due to a corner case of the HTX API.

It is possible to have an empty HTX message with a parsing/internal
error. In that case, the underlying buffer remains full. It is mandatory to
prevent any buffer release and be sure the error will be handeled.

On the other end, at several places, when data must be transfer from an HTX
message to another one, we try to swap underlying buffers instead of
performing a bloc-per-bloc copy. To do so, we rely on b_xfer() function. One
condition is that the destination message must be empty. And here is the
issue. The HTX message can be empty but the buffer can also be full because
an HTX error was triggered earlier and not handled yet. In that case,
attempting to call b_xfer() leads to a crash because the destination buffer
is full. It is not expected to call b_xfer() if there is not enough space in
the destination buffer.

So, it appears the HTX API should be improved/fixed but first of all, the
bug must be fixed. Especially because stable versions are also affected. The
htx_is_empty_noerr() function was added to know if a HTX message is empty
and no error was reported on it. And this function is now used, instead of
htx_is_empty(), to know if we can safely swap the underlying buffers or not.

the FCGI, H2 and QUIC multiplexers are concerned. The HTTP client and the
applet API were also fixed while it seems harder to trigger the bug at these
places.

The fix must be backported to all supported versions.

diff --git a/include/haproxy/htx.h b/include/haproxy/htx.h
index 6497e8337a4150e6a92a2dc64967de4f8b7e9ec0..336db649f5f61c1a20aeb3c302a6fa4ad770d995 100644 (file)
--- a/include/haproxy/htx.h
+++ b/include/haproxy/htx.h
@@ -776,6 +776,14 @@ static inline int htx_is_not_empty(const struct htx *htx)
        return (htx->head != -1);
 }
 
+/* Return 1 if the message is empty and there is no parsing/internal error flag
+ * set. Otherwise it return 0.
+ */
+static inline int htx_is_empty_noerr(const struct htx *htx)
+{
+       return (htx_is_empty(htx) && !(htx->flags & (HTX_FL_PARSING_ERROR|HTX_FL_PROCESSING_ERROR)));
+}
+
 /* Returns 1 if no more data are expected for the message <htx>. Otherwise it
  * returns 0. Note that it is illegal to call this with htx == NULL. This
  * function relies on the HTX_FL_EOM flags. It means tunneled data are not

diff --git a/src/applet.c b/src/applet.c
index 6e51884046b02c930016d5acbd3c3f993d9a6b1b..db30df612011134213dd3c6d32f59a4a91a07835 100644 (file)
--- a/src/applet.c
+++ b/src/applet.c
@@ -497,14 +497,14 @@ size_t appctx_htx_rcv_buf(struct appctx *appctx, struct buffer *buf, size_t coun
        struct htx *buf_htx = NULL;
        size_t ret = 0;
 
-       if (htx_is_empty(appctx_htx)) {
+       if (htx_is_empty_noerr(appctx_htx)) {
                htx_to_buf(appctx_htx, &appctx->outbuf);
                goto out;
        }
 
        ret = appctx_htx->data;
        buf_htx = htx_from_buf(buf);
-       if (b_size(&appctx->outbuf) == b_size(buf) && htx_is_empty(buf_htx) && htx_used_space(appctx_htx) <= count) {
+       if (b_size(&appctx->outbuf) == b_size(buf) && htx_is_empty_noerr(buf_htx) && htx_used_space(appctx_htx) <= count) {
                htx_to_buf(buf_htx, buf);
                htx_to_buf(appctx_htx, &appctx->outbuf);
                b_xfer(buf, &appctx->outbuf, b_data(&appctx->outbuf));
@@ -599,7 +599,7 @@ size_t appctx_htx_snd_buf(struct appctx *appctx, struct buffer *buf, size_t coun
        size_t ret = 0;
 
        ret = buf_htx->data;
-       if (htx_is_empty(appctx_htx) && buf_htx->data == count) {
+       if (htx_is_empty_noerr(appctx_htx) && buf_htx->data == count) {
                htx_to_buf(appctx_htx, &appctx->inbuf);
                htx_to_buf(buf_htx, buf);
                b_xfer(&appctx->inbuf, buf, b_data(buf));

diff --git a/src/http_client.c b/src/http_client.c
index c1aa994045ce0d3ae22f07db26d570b5c6486120..1571722429a37b740b3b924b0655f33c2e692e96 100644 (file)
--- a/src/http_client.c
+++ b/src/http_client.c
@@ -605,11 +605,11 @@ void httpclient_applet_io_handler(struct appctx *appctx)
                                        hc->ops.req_payload(hc);
 
                                        hc_htx = htxbuf(&hc->req.buf);
-                                       if (htx_is_empty(hc_htx))
+                                       if (htx_is_empty_noerr(hc_htx))
                                                goto out;
 
                                        htx = htx_from_buf(outbuf);
-                                       if (htx_is_empty(htx)) {
+                                       if (htx_is_empty_noerr(htx)) {
                                                /* Here htx_to_buf() will set buffer data to 0 because
                                                 * the HTX is empty, and allow us to do an xfer.
                                                 */

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 50656eb441c24a857dfdcc4e9f5d9b9c0ff5f0d1..8cceb3e0d83640d9d687e86a1a82f8cfd7cfca2a 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -7960,7 +7960,7 @@ static size_t h2_rcv_buf(struct stconn *sc, struct buffer *buf, size_t count, in
                goto end; // may be NULL if empty
 
        h2s_htx = htx_from_buf(rxbuf);
-       if (htx_is_empty(h2s_htx) && !(h2s_htx->flags & HTX_FL_PARSING_ERROR)) {
+       if (htx_is_empty_noerr(h2s_htx)) {
                /* Here htx_to_buf() will set buffer data to 0 because
                 * the HTX is empty.
                 */
@@ -7972,7 +7972,7 @@ static size_t h2_rcv_buf(struct stconn *sc, struct buffer *buf, size_t count, in
 
        /* <buf> is empty and the message is small enough, swap the
         * buffers. */
-       if (htx_is_empty(buf_htx) && htx_used_space(h2s_htx) <= count) {
+       if (htx_is_empty_noerr(buf_htx) && htx_used_space(h2s_htx) <= count) {
                count -= h2s_htx->data;
                htx_to_buf(buf_htx, buf);
                htx_to_buf(h2s_htx, rxbuf);

diff --git a/src/qcm_http.c b/src/qcm_http.c
index c2f0ae1ebcc6f243ad74455bfdc21403e18bd3ff..f028cb992f37e2ca9d5456197c94334f3bc173bb 100644 (file)
--- a/src/qcm_http.c
+++ b/src/qcm_http.c
@@ -22,7 +22,7 @@ size_t qcs_http_rcv_buf(struct qcs *qcs, struct buffer *buf, size_t count,
 
        *fin = 0;
        qcs_htx = htx_from_buf(&qcs->rx.app_buf);
-       if (htx_is_empty(qcs_htx)) {
+       if (htx_is_empty_noerr(qcs_htx)) {
                /* Set buffer data to 0 as HTX is empty. */
                htx_to_buf(qcs_htx, &qcs->rx.app_buf);
                goto end;
@@ -31,7 +31,7 @@ size_t qcs_http_rcv_buf(struct qcs *qcs, struct buffer *buf, size_t count,
        ret = qcs_htx->data;
 
        cs_htx = htx_from_buf(buf);
-       if (htx_is_empty(cs_htx) && htx_used_space(qcs_htx) <= count) {
+       if (htx_is_empty_noerr(cs_htx) && htx_used_space(qcs_htx) <= count) {
                /* EOM will be copied to cs_htx via b_xfer(). */
                if ((qcs_htx->flags & HTX_FL_EOM) &&
                    !(qcs->flags & QC_SF_EOI_SUSPENDED)) {