From 516509c54fd3f95fd31cd455aa24fb84dae751de Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Tue, 15 Jan 2019 17:05:45 +0500 Subject: [PATCH 1/6] fix #1147 : init frag variable before use. --- tempesta_fw/msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tempesta_fw/msg.c b/tempesta_fw/msg.c index 17da9888f..1797dcade 100644 --- a/tempesta_fw/msg.c +++ b/tempesta_fw/msg.c @@ -36,7 +36,7 @@ tfw_msg_write(TfwMsgIter *it, const TfwStr *data) { char *p; const TfwStr *c, *end; - skb_frag_t *frag; + skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; unsigned int c_off = 0, f_size, c_size, f_room, n_copy; BUG_ON(TFW_STR_DUP(data)); From 3ed0fd90fb73df8f5e02755e7ad69dc6c2e9fa86 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Thu, 24 Jan 2019 15:53:50 +0500 Subject: [PATCH 2/6] Keep arguments of ss_skb_frag_next syncronized The function receives TfwMsgIter as argumrnt (as the arguments actually), The 'skb' and 'f' point to specific fragment of the skb. If the condition is false, then TfwMsgIter->skb and TfwMsgIter->frag may point to unexisting skb fragment. --- tempesta_fw/ss_skb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tempesta_fw/ss_skb.h b/tempesta_fw/ss_skb.h index c7a281e0b..8e207ad7e 100644 --- a/tempesta_fw/ss_skb.h +++ b/tempesta_fw/ss_skb.h @@ -129,9 +129,9 @@ ss_skb_frag_next(struct sk_buff **skb, const struct sk_buff *skb_head, int *f) } *skb = (*skb)->next; + *f = 0; if (*skb == skb_head || !skb_shinfo(*skb)->nr_frags) return NULL; - *f = 0; return &skb_shinfo(*skb)->frags[0]; } From f763b9dee0960795b6f8343379301890cc5965f7 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Thu, 24 Jan 2019 16:08:13 +0500 Subject: [PATCH 3/6] Remove dead stores and dengerous initialisation --- tempesta_fw/msg.c | 47 +++++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/tempesta_fw/msg.c b/tempesta_fw/msg.c index 1797dcade..3f6405ddd 100644 --- a/tempesta_fw/msg.c +++ b/tempesta_fw/msg.c @@ -34,20 +34,22 @@ int tfw_msg_write(TfwMsgIter *it, const TfwStr *data) { - char *p; const TfwStr *c, *end; - skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; - unsigned int c_off = 0, f_size, c_size, f_room, n_copy; BUG_ON(TFW_STR_DUP(data)); + if (WARN_ON_ONCE(it->frag >= skb_shinfo(it->skb)->nr_frags)) + return -E2BIG; + TFW_STR_FOR_EACH_CHUNK(c, data, end) { + char *p; + unsigned int c_off = 0, c_size, f_room, n_copy; this_chunk: - if (!frag) - return -E2BIG; c_size = c->len - c_off; if (it->frag >= 0) { - frag = &skb_shinfo(it->skb)->frags[it->frag]; + unsigned int f_size; + skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; + f_size = skb_frag_size(frag); f_room = PAGE_SIZE - frag->page_offset - f_size; p = (char *)skb_frag_address(frag) + f_size; @@ -62,24 +64,25 @@ tfw_msg_write(TfwMsgIter *it, const TfwStr *data) memcpy_fast(p, c->data + c_off, n_copy); - if (c_size < f_room) { - /* - * The chunk fits in the SKB fragment with room - * to spare. Stay in the same SKB fragment, switch - * to next chunk of the string. - */ - c_off = 0; - } else { - frag = ss_skb_frag_next(&it->skb, it->skb_head, - &it->frag); + /* + * The chunk occupied all the spare space in the SKB fragment, + * switch to the next fragment. + */ + if (c_size >= f_room) { + skb_frag_t *frag = ss_skb_frag_next(&it->skb, + it->skb_head, + &it->frag); + if (unlikely(!frag) + && ((c_size != f_room) || (c + 1 < end))) + { + return -E2BIG; + } /* - * If all data from the chunk has been copied, - * then switch to the next chunk. Otherwise, - * stay in the current chunk. + * Not all data from the chunk has been copied, + * stay in the current chunk and copy the rest to the + * next fragment. */ - if (c_size == f_room) { - c_off = 0; - } else { + if (c_size != f_room) { c_off += n_copy; goto this_chunk; } From 3b930c154778165abd9fe8fec3f0615b531bc825 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Wed, 6 Feb 2019 18:54:35 +0500 Subject: [PATCH 4/6] Remove extra apm update for HM responses The issue was noticed in comment https://github.com/tempesta-tech/tempesta/pull/1169#discussion_r253041405 Anyway tfw_apm_update() in tfw_http_hm_drop_resp() doesn't look good. --- tempesta_fw/http.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tempesta_fw/http.c b/tempesta_fw/http.c index 2d965ee1e..32c848348 100644 --- a/tempesta_fw/http.c +++ b/tempesta_fw/http.c @@ -2787,8 +2787,6 @@ tfw_http_hm_drop_resp(TfwHttpResp *resp) TfwHttpReq *req = resp->req; tfw_connection_unlink_msg(resp->conn); - tfw_apm_update(((TfwServer *)resp->conn->peer)->apmref, - resp->jrxtstamp, resp->jrxtstamp - req->jtxtstamp); tfw_http_conn_msg_free((TfwHttpMsg *)resp); tfw_http_msg_free((TfwHttpMsg *)req); } From dc9558f2bda65216947125513659d79fb05d1639 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Wed, 6 Feb 2019 19:02:19 +0500 Subject: [PATCH 5/6] Fix ignored tfw_msg_write() error --- tempesta_fw/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tempesta_fw/http.c b/tempesta_fw/http.c index 32c848348..b14cd7b20 100644 --- a/tempesta_fw/http.c +++ b/tempesta_fw/http.c @@ -438,7 +438,7 @@ tfw_http_prep_redirect(TfwHttpMsg *resp, unsigned short status, TfwStr *rmark, tfw_http_prep_date(__TFW_STR_CH(&h_common_1, 1)->data); ret = tfw_msg_write(&it, rh); - ret = tfw_msg_write(&it, &h_common_1); + ret |= tfw_msg_write(&it, &h_common_1); /* * HTTP/1.0 may have no host part, so we create relative URI. * See RFC 1945 9.3 and RFC 7231 7.1.2. From 96b8933ed99098a3cc9d06c15c6759d280171ab3 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Fri, 8 Feb 2019 15:00:09 +0500 Subject: [PATCH 6/6] Add warning when the ss_skb_frag_next() fails for precalculated message --- tempesta_fw/msg.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tempesta_fw/msg.c b/tempesta_fw/msg.c index 3f6405ddd..6ad3c2505 100644 --- a/tempesta_fw/msg.c +++ b/tempesta_fw/msg.c @@ -72,8 +72,9 @@ tfw_msg_write(TfwMsgIter *it, const TfwStr *data) skb_frag_t *frag = ss_skb_frag_next(&it->skb, it->skb_head, &it->frag); - if (unlikely(!frag) - && ((c_size != f_room) || (c + 1 < end))) + if (WARN_ON_ONCE(!frag + && ((c_size != f_room) + || (c + 1 < end)))) { return -E2BIG; }