MINOR: quic: do not modify offset node if quic_rx_strm_frm in tree
qc_rx_strm_frm_cpy is unsafe because it updates the offset field of the
frame. This is not safe as the frame is inserted in the tree when
calling this function and offset serves as the key node.
To fix this, the API is modified so that qc_rx_strm_frm_cpy does not
update the frame parameter. The caller is responsible to update
offset/length in case of a partial copy.
The impact of this bug is not known. It can only happened with received
STREAM frames out-of-order. This might be triggered with large h3 POST
requests.
diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 0048f7b..b052509 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -1964,28 +1964,29 @@
}
/* Copy as most as possible STREAM data from <strm_frm> into <buf> buffer.
- * Also update <strm_frm> frame to reflect the data which have been consumed.
+ *
+ * Note that <strm_frm> is not updated as it is implied that the frame may be
+ * present in a tree and offset node is used as the key. The caller should
+ * update offset/lenght of the frame after the function call.
+ *
+ * Return the total count of copied bytes.
*/
static size_t qc_rx_strm_frm_cpy(struct buffer *buf,
struct quic_rx_strm_frm *strm_frm)
{
- size_t ret;
+ size_t flen = strm_frm->len;
+ size_t ret = 0;
+ size_t try;
ret = 0;
- while (strm_frm->len) {
- size_t try;
+ while (flen && (try = b_contig_space(buf))) {
+ if (try > flen)
+ try = flen;
- try = b_contig_space(buf);
- if (!try)
- break;
-
- if (try > strm_frm->len)
- try = strm_frm->len;
- memcpy(b_tail(buf), strm_frm->data, try);
- strm_frm->len -= try;
- strm_frm->offset_node.key += try;
+ memcpy(b_tail(buf), strm_frm->data + ret, try);
b_add(buf, try);
ret += try;
+ flen -= try;
}
return ret;
@@ -2014,6 +2015,10 @@
/* If there is remaining data in this frame
* this is because the destination buffer is full.
*/
+ eb64_delete(&frm->offset_node);
+ frm->offset_node.key += ret;
+ frm->len -= ret;
+ eb64_insert(&qcs->rx.frms, &frm->offset_node);
break;
}