Skip to content
Permalink
Browse files

BUG/MAJOR: mux-h1: Don't crush trash chunk area when outgoing message…

… is formatted

When an outgoing HTX message is formatted before sending it, a trash chunk is
used to do the formatting. Its content is then copied into the output buffer of
the H1 connection. There are some tricks to avoid this last copy. First, if
possible we perform a zero-copy by swapping the area of the HTX buffer with the
one of the output buffer. If zero-copy is not possible, but if the output buffer
is empty, we don't use a trash chunk. To do so, we change the area of the trash
chunk to point on the one of the output buffer. But it is terribly wrong. Trash
chunks are global variables, allocated statically. If the area is changed, the
old one is lost. Worst, the area of the output buffer is dynamically allocated,
so it is released when emptied, leaving the trash chunk with a freed area (in
fact, it is a bit more complicated because buffers are allocated from a memory
pool).

So, honestly, I don't know why we never experienced any problem because this bug
till now. To fix it, we still use a temporary buffer, but we assign it to a
trash chunk only when other solutions were excluded. This way, we never
overwrite the area of a trash chunk.

This patch must be backported to 2.0 and 1.9.
  • Loading branch information...
capflam committed Jun 25, 2019
1 parent 2bce046 commit c2518a53aed70b373edc5c169cc3a02ed2398bba
Showing with 21 additions and 22 deletions.
  1. +21 −22 src/mux_h1.c
@@ -1478,7 +1478,7 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
struct h1m *h1m;
struct htx *chn_htx;
struct htx_blk *blk;
struct buffer *tmp;
struct buffer tmp;
size_t total = 0;
int errflag;

@@ -1506,8 +1506,6 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
/* the htx is non-empty thus has at least one block */
blk = htx_get_head_blk(chn_htx);

tmp = get_trash_chunk();

/* Perform some optimizations to reduce the number of buffer copies.
* First, if the mux's buffer is empty and the htx area contains
* exactly one data block of the same size as the requested count,
@@ -1525,14 +1523,13 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
* the HTX blocks.
*/
if (!b_data(&h1c->obuf)) {
h1c->obuf.head = sizeof(struct htx) + blk->addr;

if (chn_htx->used == 1 &&
htx_get_blk_type(blk) == HTX_BLK_DATA &&
htx_get_blk_value(chn_htx, blk).len == count) {
void *old_area = h1c->obuf.area;

h1c->obuf.area = buf->area;
h1c->obuf.head = sizeof(struct htx) + blk->addr;
h1c->obuf.data = count;

buf->area = old_area;
@@ -1550,11 +1547,13 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
total += count;
goto out;
}
tmp->area = h1c->obuf.area + h1c->obuf.head;
tmp.area = h1c->obuf.area + h1c->obuf.head;
}
else
tmp.area = trash.area;

tmp->size = b_room(&h1c->obuf);

tmp.data = 0;
tmp.size = b_room(&h1c->obuf);
while (count && !(h1s->flags & errflag) && blk) {
struct htx_sl *sl;
struct ist n, v;
@@ -1579,7 +1578,7 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
sl = htx_get_blk_ptr(chn_htx, blk);
h1s->meth = sl->info.req.meth;
h1_parse_req_vsn(h1m, sl);
if (!htx_reqline_to_h1(sl, tmp))
if (!htx_reqline_to_h1(sl, &tmp))
goto copy;
h1m->flags |= H1_MF_XFER_LEN;
if (sl->flags & HTX_SL_F_BODYLESS)
@@ -1593,7 +1592,7 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
sl = htx_get_blk_ptr(chn_htx, blk);
h1s->status = sl->info.res.status;
h1_parse_res_vsn(h1m, sl);
if (!htx_stline_to_h1(sl, tmp))
if (!htx_stline_to_h1(sl, &tmp))
goto copy;
if (sl->flags & HTX_SL_F_XFER_LEN)
h1m->flags |= H1_MF_XFER_LEN;
@@ -1627,7 +1626,7 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
goto skip_hdr;
}

if (!htx_hdr_to_h1(n, v, tmp))
if (!htx_hdr_to_h1(n, v, &tmp))
goto copy;
skip_hdr:
h1m->state = H1_MSG_HDR_L2_LWS;
@@ -1646,7 +1645,7 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
v = ist("");
h1_process_output_conn_mode(h1s, h1m, &v);
if (v.len) {
if (!htx_hdr_to_h1(n, v, tmp))
if (!htx_hdr_to_h1(n, v, &tmp))
goto copy;
}
h1s->flags |= H1S_F_HAVE_O_CONN;
@@ -1660,12 +1659,12 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
(h1m->flags & (H1_MF_VER_11|H1_MF_RESP|H1_MF_CLEN|H1_MF_CHNK|H1_MF_XFER_LEN)) ==
(H1_MF_VER_11|H1_MF_RESP|H1_MF_XFER_LEN))) {
/* chunking needed but header not seen */
if (!chunk_memcat(tmp, "transfer-encoding: chunked\r\n", 28))
if (!chunk_memcat(&tmp, "transfer-encoding: chunked\r\n", 28))
goto copy;
h1m->flags |= H1_MF_CHNK;
}

if (!chunk_memcat(tmp, "\r\n", 2))
if (!chunk_memcat(&tmp, "\r\n", 2))
goto copy;

if (!(h1m->flags & H1_MF_RESP) && h1s->meth == HTTP_METH_CONNECT) {
@@ -1691,21 +1690,21 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
if (type == HTX_BLK_EOM) {
/* Chunked message without explicit trailers */
if (h1m->flags & H1_MF_CHNK) {
if (!chunk_memcat(tmp, "0\r\n\r\n", 5))
if (!chunk_memcat(&tmp, "0\r\n\r\n", 5))
goto copy;
}
goto done;
}
else if (type == HTX_BLK_EOT || type == HTX_BLK_TLR) {
if (!chunk_memcat(tmp, "0\r\n", 3))
if (!chunk_memcat(&tmp, "0\r\n", 3))
goto copy;
goto trailers;
}
else if (type != HTX_BLK_DATA)
goto error;
v = htx_get_blk_value(chn_htx, blk);
v.len = vlen;
if (!htx_data_to_h1(v, tmp, !!(h1m->flags & H1_MF_CHNK)))
if (!htx_data_to_h1(v, &tmp, !!(h1m->flags & H1_MF_CHNK)))
goto copy;
break;

@@ -1717,13 +1716,13 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
trailers:
h1m->state = H1_MSG_TRAILERS;
if (type == HTX_BLK_EOT) {
if (!chunk_memcat(tmp, "\r\n", 2))
if (!chunk_memcat(&tmp, "\r\n", 2))
goto copy;
}
else { // HTX_BLK_TLR
n = htx_get_blk_name(chn_htx, blk);
v = htx_get_blk_value(chn_htx, blk);
if (!htx_hdr_to_h1(n, v, tmp))
if (!htx_hdr_to_h1(n, v, &tmp))
goto copy;
}
break;
@@ -1760,10 +1759,10 @@ static size_t h1_process_output(struct h1c *h1c, struct buffer *buf, size_t coun
/* when the output buffer is empty, tmp shares the same area so that we
* only have to update pointers and lengths.
*/
if (tmp->area == h1c->obuf.area + h1c->obuf.head)
h1c->obuf.data = tmp->data;
if (tmp.area == h1c->obuf.area + h1c->obuf.head)
h1c->obuf.data = tmp.data;
else
b_putblk(&h1c->obuf, tmp->area, tmp->data);
b_putblk(&h1c->obuf, tmp.area, tmp.data);

htx_to_buf(chn_htx, buf);
out:

0 comments on commit c2518a5

Please sign in to comment.
You can’t perform that action at this time.