From: Amaury Denoyelle Date: Mon, 13 May 2024 14:01:08 +0000 (+0200) Subject: BUG/MINOR: qpack: fix error code reported on QPACK decoding failure X-Git-Tag: v3.0-dev12~44 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=86aafd02365a132327a7a090a2a3f7c972785c21;p=thirdparty%2Fhaproxy.git 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 8ea7689682..df15b1d28c 100644 --- 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 7e681cec05..b5954ff45a 100644 --- 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 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 a H3 HEADERS frame of length . Data are copied * in a local HTX buffer and transfer to the stream connector layer. 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 7a8726f2c0..f96b17b2d3 100644 --- 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; +}