From b666c6b26eae3f17eee058eb6bcc9fe1b1c304d2 Mon Sep 17 00:00:00 2001
From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Tue, 14 Jun 2022 17:17:07 +0200
Subject: [PATCH] MINOR: qpack: improve decoding function

Adjust decoding loop by using temporary ist for header name and value.
The header is inserted at the end of an iteration, which guarantee that
we do not insert name only in the list in case of an error on value
decoding. This also helps the function readability by centralizing the
LIST insert operation.

The return value of the decoding function is also changed. Now on
success the number of headers inserted in the input list is returned.
This change as no impact as success value is not used by the caller.
This is mainly done to have a behavior similar to hpack decoding
function.
---
 src/qpack-dec.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/qpack-dec.c b/src/qpack-dec.c
index 4ee466d889..bb960b4b94 100644
--- a/src/qpack-dec.c
+++ b/src/qpack-dec.c
@@ -181,12 +181,13 @@ static int qpack_decode_fs_pfx(uint64_t *enc_ric, uint64_t *db, int *sign_bit,
  * a storage for some elements pointing into it. An end marker is inserted at
  * the end of the list with empty strings as name/value.
  *
- * Returns 0 on success. In case of error, a negative code QPACK_ERR_* is
- * returned.
+ * Returns the number of headers inserted into list excluding the end marker.
+ * In case of error, a negative code QPACK_ERR_* is returned.
  */
 int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
                     struct http_hdr *list, int list_size)
 {
+	struct ist name, value;
 	uint64_t enc_ric, db;
 	int s;
 	unsigned int efl_type;
@@ -299,8 +300,10 @@ int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
 				goto out;
 			}
 
-			if (t)
-				list[hdr_idx++] = qpack_sht[index];
+			if (t) {
+				name = qpack_sht[index].n;
+				value = qpack_sht[index].v;
+			}
 
 			qpack_debug_printf(stderr,  " t=%d index=%llu", !!t, (unsigned long long)index);
 		}
@@ -320,7 +323,7 @@ int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
 			}
 
 			if (t)
-				list[hdr_idx] = qpack_sht[index];
+				name = qpack_sht[index].n;
 
 			qpack_debug_printf(stderr, " n=%d t=%d index=%llu", !!n, !!t, (unsigned long long)index);
 			h = *raw & 0x80;
@@ -352,10 +355,10 @@ int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
 				qpack_debug_printf(stderr, " [name huff %d->%d '%s']", (int)length, (int)nlen, trash);
 				/* makes an ist from tmp storage */
 				b_add(tmp, nlen);
-				list[hdr_idx].v = ist2(trash, nlen);
+				value = ist2(trash, nlen);
 			}
 			else {
-				list[hdr_idx].v = ist2(raw, length);
+				value = ist2(raw, length);
 			}
 
 			if (len < length) {
@@ -366,7 +369,6 @@ int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
 
 			raw += length;
 			len -= length;
-			++hdr_idx;
 		}
 		else if (efl_type & QPACK_LFL_WLN_BIT) {
 			/* Literal field line with literal name */
@@ -412,14 +414,15 @@ int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
 				qpack_debug_printf(stderr, " [name huff %d->%d '%s']", (int)name_len, (int)nlen, trash);
 				/* makes an ist from tmp storage */
 				b_add(tmp, nlen);
-				list[hdr_idx].n = ist2(trash, nlen);
+				name = ist2(trash, nlen);
 			}
 			else {
-				list[hdr_idx].n = ist2(raw, name_len);
+				name = ist2(raw, name_len);
 			}
 
 			raw += name_len;
 			len -= name_len;
+
 			hvalue = *raw & 0x80;
 			value_len = qpack_get_varint(&raw, &len, 7);
 			if (len == (uint64_t)-1) {
@@ -456,17 +459,20 @@ int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
 				qpack_debug_printf(stderr, " [name huff %d->%d '%s']", (int)value_len, (int)nlen, trash);
 				/* makes an ist from tmp storage */
 				b_add(tmp, nlen);
-				list[hdr_idx].v = ist2(trash, nlen);
+				value = ist2(trash, nlen);
 			}
 			else {
-				list[hdr_idx].v = ist2(raw, value_len);
+				value = ist2(raw, value_len);
 			}
 
 			raw += value_len;
 			len -= value_len;
-
-			++hdr_idx;
 		}
+
+		list[hdr_idx].n = name;
+		list[hdr_idx].v = value;
+		++hdr_idx;
+
 		qpack_debug_printf(stderr, "\n");
 	}
 
@@ -478,6 +484,7 @@ int qpack_decode_fs(const unsigned char *raw, size_t len, struct buffer *tmp,
 
 	/* put an end marker */
 	list[hdr_idx].n = list[hdr_idx].v = IST_NULL;
+	ret = hdr_idx;
 
  out:
 	qpack_debug_printf(stderr, "-- done: ret=%d\n", ret);