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 16ce85a1fbd02f47d5a94bf31c0b41121e50b4bb..bc49ef2b43aa66b7eb69d269e316fd951c47bb76 100644 (file)
--- 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 <rxbuf> buffer. The frame type is stored in

diff --git a/src/qpack-dec.c b/src/qpack-dec.c
index e1c14716dc7422f49de89da6b6ddf888bb8eb4aa..2f9d891a7aa4a2876f3e77b80877bded9133707e 100644 (file)
--- 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 <buf>,
+ * whose first byte carries <prefix_bits> 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 <enc_ric> and <db> two varints.