From: Frederic Lecaille Date: Tue, 1 Sep 2026 14:57:42 +0000 (+0200) Subject: BUG/MINOR: qpack: encoder and decoder stream data is never consumed X-Git-Tag: v3.5-dev6~16 X-Git-Url: https://git.kaiwu.me/?a=commitdiff_plain;h=01cdc8083aecfcbf736efae31c442d3c5b02002d;p=haproxy.git BUG/MINOR: qpack: encoder and decoder stream data is never consumed qpack_decode_enc() and qpack_decode_dec() always return 0, which h3_parse_uni_stream_no_h3() propagates as a consumed byte count. qcs_consume() is thus never called: the data stays in the Rx buffer and the flow control credit is never released, so both QPACK unidirectional streams stall for good on the first byte the peer sends. On the encoder stream that byte is 0x20, which a conformant peer sends to announce a null dynamic table capacity. Report the number of consumed bytes instead, and skip the stream ID of Section Acknowledgment and Stream Cancellation, which haproxy has no use for. The Insert Count Increment test only caught 0x00 to 0x0f and now covers its whole range. The encoder stream insert instructions are rejected, as haproxy advertises a null dynamic table capacity. This bug was introduced by commit b4672fb6f ("MINOR: qpack: Add QPACK compression"). This should be backported as far as 2.6. --- diff --git a/src/h3.c b/src/h3.c index 16ce85a1f..bc49ef2b4 100644 --- a/src/h3.c +++ b/src/h3.c @@ -307,18 +307,17 @@ static ssize_t h3_init_uni_stream(struct h3c *h3c, struct qcs *qcs, static ssize_t h3_parse_uni_stream_no_h3(struct qcs *qcs, struct buffer *b, int fin) { struct h3s *h3s = qcs->ctx; + int ret = -1; /* Function reserved to non-HTTP/3 unidirectional streams. */ BUG_ON(!quic_stream_is_uni(qcs->id) || !(h3s->flags & H3_SF_UNI_NO_H3)); switch (h3s->type) { case H3S_T_QPACK_DEC: - if (qpack_decode_dec(b, fin, qcs)) - return -1; + ret = qpack_decode_dec(b, fin, qcs); break; case H3S_T_QPACK_ENC: - if (qpack_decode_enc(b, fin, qcs)) - return -1; + ret = qpack_decode_enc(b, fin, qcs); break; case H3S_T_UNKNOWN: default: @@ -326,8 +325,7 @@ static ssize_t h3_parse_uni_stream_no_h3(struct qcs *qcs, struct buffer *b, int ABORT_NOW(); } - /* TODO adjust return code */ - return 0; + return ret; } /* Decode a H3 frame header from buffer. The frame type is stored in diff --git a/src/qpack-dec.c b/src/qpack-dec.c index e1c14716d..2f9d891a7 100644 --- a/src/qpack-dec.c +++ b/src/qpack-dec.c @@ -98,9 +98,30 @@ too_short: return 0; } +/* Return the length in bytes of the QPACK prefix integer at the head of , + * whose first byte carries significant bits. Returns 0 if the + * integer is not complete yet, or a negative value if it is too long to be a + * valid one. + */ +static int qpack_prefix_int_len(const struct buffer *buf, int prefix_bits) +{ + /* One prefix byte plus the nine continuations of a 62-bit value. */ + unsigned char tmp[10]; + const unsigned char *raw = tmp; + uint64_t len = MIN(sizeof tmp, b_data(buf)); + + b_getblk(buf, (char *)tmp, len, 0); + qpack_get_varint(&raw, &len, prefix_bits); + + /* A truncated and a too large integer are reported the same way. */ + if (len == (uint64_t)-1) + return b_data(buf) < sizeof tmp ? 0 : -1; + + return raw - tmp; +} + /* Decode an encoder stream. - * - * Returns 0 on success else non-zero. + * Returns the number of consumed bytes or a negative error code. */ int qpack_decode_enc(struct buffer *buf, int fin, void *ctx) { @@ -130,16 +151,7 @@ int qpack_decode_enc(struct buffer *buf, int fin, void *ctx) } inst = (unsigned char)*b_head(buf) & QPACK_ENC_INST_BITMASK; - if (inst == QPACK_ENC_INST_DUP) { - /* Duplicate */ - } - else if (inst & QPACK_ENC_INST_IWNR_BIT) { - /* Insert With Name Reference */ - } - else if (inst & QPACK_ENC_INST_IWLN_BIT) { - /* Insert with literal name */ - } - else if (inst & QPACK_ENC_INST_SDTC_BIT) { + if (inst & QPACK_ENC_INST_SDTC_BIT) { /* Set dynamic table capacity */ int capacity = *b_head(buf) & 0x1f; @@ -155,20 +167,26 @@ int qpack_decode_enc(struct buffer *buf, int fin, void *ctx) return -1; } + return 1; } - return 0; + /* haproxy advertises a null dynamic table capacity, so the peer must not + * insert anything. + */ + qcc_set_error(qcs->qcc, QPACK_ERR_ENCODER_STREAM_ERROR, 1, + muxc_tevt_type_proto_err); + return -1; } /* Decode an decoder stream. - * - * Returns 0 on success else non-zero. + * Returns the number of consumed bytes or a negative error code. */ int qpack_decode_dec(struct buffer *buf, int fin, void *ctx) { struct qcs *qcs = ctx; size_t len; unsigned char inst; + int rdlen; /* RFC 9204 4.2. Encoder and Decoder Streams * @@ -191,8 +209,8 @@ int qpack_decode_dec(struct buffer *buf, int fin, void *ctx) return 0; } - inst = (unsigned char)*b_head(buf) & QPACK_DEC_INST_BITMASK; - if (inst == QPACK_DEC_INST_ICINC) { + inst = (unsigned char)*b_head(buf); + if (!(inst & (QPACK_DEC_INST_SACK | QPACK_DEC_INST_SCCL))) { /* Insert count increment */ /* RFC 9204 4.4.3. Insert Count Increment @@ -208,14 +226,23 @@ int qpack_decode_dec(struct buffer *buf, int fin, void *ctx) muxc_tevt_type_proto_err); return -1; } - else if (inst & QPACK_DEC_INST_SACK) { - /* Section Acknowledgment */ - } - else if (inst & QPACK_DEC_INST_SCCL) { - /* Stream cancellation */ + + /* Section Acknowledgment and Stream Cancellation both carry a stream ID + * which is of no use here, as haproxy encodes every field section with a + * null Required Insert Count. Skip them. + */ + if (inst & QPACK_DEC_INST_SACK) + rdlen = qpack_prefix_int_len(buf, 7); + else + rdlen = qpack_prefix_int_len(buf, 6); + + if (rdlen < 0) { + qcc_set_error(qcs->qcc, QPACK_ERR_DECODER_STREAM_ERROR, 1, + muxc_tevt_type_proto_err); + return -1; } - return 0; + return rdlen; } /* Decode a field section prefix made of and two varints.