BUG/MINOR: qpack: fix error code reported on QPACK decoding failure

qpack_decode_fs() is used to decode QPACK field section on HTTP/3
headers parsing. Its return value is incoherent as it returns either
QPACK_DECOMPRESSION_FAILED defined in RFC 9204 or any other internal
values defined in qpack-dec.h. On failure, such return code is reused by
HTTP/3 layer to be reported via a CONNECTION_CLOSE frame. This is
incorrect if an internal error values was reported as it is not defined
by any specification.

Fir return values of qpack_decode_fs() in two ways. Firstly, fix invalid
usages of QPACK_DECOMPRESSION_FAILED when decoded content is too large
for the correct internal error QPACK_ERR_TOO_LARGE.

Secondly, adjust qpack_decode_fs() API to only returns internal code
values. A new internal enum QPACK_ERR_DECOMP is defined to replace
QPACK_DECOMPRESSION_FAILED. Caller is responsible to convert it to a
suitable error value. For other internal values, H3_INTERNAL_ERROR is
used. This is done through a set of convert functions.

This should be backported up to 2.6. Note that trailers are not
supported in 2.6 so chunk related to h3_trailers_to_htx() can be safely
skipped.

diff --git a/include/haproxy/qpack-dec.h b/include/haproxy/qpack-dec.h
index 8ea7689682e274cb9604986df21234b0ebff5b6e..df15b1d28ca39c5cac3edeb8ce318630a47bb9f9 100644 (file)
--- a/include/haproxy/qpack-dec.h
+++ b/include/haproxy/qpack-dec.h
@@ -31,6 +31,7 @@ struct http_hdr;
  */
 enum {
        QPACK_ERR_NONE = 0,  /* no error */
+       QPACK_ERR_DECOMP,    /* corresponds to RFC 9204 decompression error */
        QPACK_ERR_RIC,       /* cannot decode Required Insert Count prefix field */
        QPACK_ERR_DB,        /* cannot decode Delta Base prefix field */
        QPACK_ERR_TRUNCATED, /* truncated stream */
@@ -50,4 +51,6 @@ int qpack_decode_fs(const unsigned char *buf, uint64_t len, struct buffer *tmp,
 int qpack_decode_enc(struct buffer *buf, int fin, void *ctx);
 int qpack_decode_dec(struct buffer *buf, int fin, void *ctx);
 
+int qpack_err_decode(const int value);
+
 #endif /* _HAPROXY_QPACK_DEC_H */

diff --git a/src/h3.c b/src/h3.c
index 7e681cec05809ecb40c2090ba59a81d433bf5aea..b5954ff45a821817815a6e2c64b9854129930c01 100644 (file)
--- a/src/h3.c
+++ b/src/h3.c
@@ -131,7 +131,7 @@ static uint64_t h3_settings_max_field_section_size = QUIC_VARINT_8_BYTE_MAX; /*
 struct h3c {
        struct qcc *qcc;
        struct qcs *ctrl_strm; /* Control stream */
-       enum h3_err err;
+       int err;
        uint32_t flags;
 
        /* Settings */
@@ -504,6 +504,12 @@ static int h3_set_authority(struct qcs *qcs, struct ist *auth, const struct ist
        return 0;
 }
 
+/* Return <value> as is or H3_INTERNAL_ERROR if negative. Useful to prepare a standard error code. */
+static int h3_err(const int value)
+{
+       return value >= 0 ? value : H3_INTERNAL_ERROR;
+}
+
 /* Parse from buffer <buf> a H3 HEADERS frame of length <len>. Data are copied
  * in a local HTX buffer and transfer to the stream connector layer. <fin> must be
  * set if this is the last data to transfer from this stream.
@@ -560,7 +566,7 @@ static ssize_t h3_headers_to_htx(struct qcs *qcs, const struct buffer *buf,
                            list, sizeof(list) / sizeof(list[0]));
        if (ret < 0) {
                TRACE_ERROR("QPACK decoding error", H3_EV_RX_FRAME|H3_EV_RX_HDR, qcs->qcc->conn, qcs);
-               h3c->err = -ret;
+               h3c->err = h3_err(qpack_err_decode(ret));
                len = -1;
                goto out;
        }
@@ -939,7 +945,7 @@ static ssize_t h3_trailers_to_htx(struct qcs *qcs, const struct buffer *buf,
                            list, sizeof(list) / sizeof(list[0]));
        if (ret < 0) {
                TRACE_ERROR("QPACK decoding error", H3_EV_RX_FRAME|H3_EV_RX_HDR, qcs->qcc->conn, qcs);
-               h3c->err = -ret;
+               h3c->err = h3_err(qpack_err_decode(ret));
                len = -1;
                goto out;
        }

diff --git a/src/qpack-dec.c b/src/qpack-dec.c
index 7a8726f2c02a87f498f94a0c6413a888e0becdd4..f96b17b2d3c3130e8317f56fb316b4384f449bcf 100644 (file)
--- a/src/qpack-dec.c
+++ b/src/qpack-dec.c
@@ -316,7 +316,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                         * Count (Section 4.5.1), it MUST treat this as a connection error of
                         * type QPACK_DECOMPRESSION_FAILED.
                         */
-                       return -QPACK_DECOMPRESSION_FAILED;
+                       return -QPACK_ERR_DECOMP;
                }
                else if (efl_type == QPACK_IFL_WPBI) {
                        /* Indexed field line with post-base index
@@ -344,7 +344,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                         * Count (Section 4.5.1), it MUST treat this as a connection error of
                         * type QPACK_DECOMPRESSION_FAILED.
                         */
-                       return -QPACK_DECOMPRESSION_FAILED;
+                       return -QPACK_ERR_DECOMP;
                }
                else if (efl_type & QPACK_IFL_BIT) {
                        /* Indexed field line */
@@ -375,7 +375,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                                 *
                                 * TODO adjust this when dynamic table support is implemented.
                                 */
-                               return -QPACK_DECOMPRESSION_FAILED;
+                               return -QPACK_ERR_DECOMP;
                        }
 
                        qpack_debug_printf(stderr,  " t=%d index=%llu", !!static_tbl, (unsigned long long)index);
@@ -409,7 +409,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                                 *
                                 * TODO adjust this when dynamic table support is implemented.
                                 */
-                               return -QPACK_DECOMPRESSION_FAILED;
+                               return -QPACK_ERR_DECOMP;
                        }
 
                        qpack_debug_printf(stderr, " n=%d t=%d index=%llu", !!n, !!static_tbl, (unsigned long long)index);
@@ -429,7 +429,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                                trash = chunk_newstr(tmp);
                                if (!trash) {
                                        qpack_debug_printf(stderr, "##ERR@%d\n", __LINE__);
-                                       ret = -QPACK_DECOMPRESSION_FAILED;
+                                       ret = -QPACK_ERR_TOO_LARGE;
                                        goto out;
                                }
                                nlen = huff_dec(raw, length, trash, tmp->size - tmp->data);
@@ -488,7 +488,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                                trash = chunk_newstr(tmp);
                                if (!trash) {
                                        qpack_debug_printf(stderr, "##ERR@%d\n", __LINE__);
-                                       ret = -QPACK_DECOMPRESSION_FAILED;
+                                       ret = -QPACK_ERR_TOO_LARGE;
                                        goto out;
                                }
                                nlen = huff_dec(raw, name_len, trash, tmp->size - tmp->data);
@@ -533,7 +533,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                                trash = chunk_newstr(tmp);
                                if (!trash) {
                                        qpack_debug_printf(stderr, "##ERR@%d\n", __LINE__);
-                                       ret = -QPACK_DECOMPRESSION_FAILED;
+                                       ret = -QPACK_ERR_TOO_LARGE;
                                        goto out;
                                }
                                nlen = huff_dec(raw, value_len, trash, tmp->size - tmp->data);
@@ -561,7 +561,7 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
                 */
                if (!name.len) {
                        qpack_debug_printf(stderr, "##ERR@%d\n", __LINE__);
-                       ret = -QPACK_DECOMPRESSION_FAILED;
+                       ret = -QPACK_ERR_DECOMP;
                        goto out;
                }
 
@@ -586,3 +586,11 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
        qpack_debug_printf(stderr, "-- done: ret=%d\n", ret);
        return ret;
 }
+
+/* Convert return value from qpack_decode_fs() to a standard error code usable
+ * in CONNECTION_CLOSE or -1 for an internal error.
+ */
+int qpack_err_decode(const int value)
+{
+       return (value == -QPACK_ERR_DECOMP) ? QPACK_DECOMPRESSION_FAILED : -1;
+}