fix(1902): correct trailer headers placement #2173

kingluo · 2024-07-17T07:34:57Z

close #1902

fw/http_frame.c

fw/http2.c

fw/cache.c

fw/http_frame.c

fw/http2.c

fw/hpack.c

fw/http_frame.c

fw/http_stream.h

fw/http_frame.c

EvgeniiMekhanik

Need fixes

fw/http_stream.h

fw/cache.c

fw/http2.c

fw/cache.c

fw/http_frame.c

fw/http_msg.h

const-t

LGTM, but need minor fixes. I have to add, that resp_hdr_set(delete/replace header) will to work with trailers. Could you please describe it on the wiki page of resp_hdr_set.

fw/cache.c

const-t · 2024-08-30T08:57:38Z

fw/cache.c

+		FOR_EACH_HDR_FIELD_FROM(field, end1, resp, TFW_HTTP_HDR_REGULAR) {
+			int hid = field - resp->h_tbl->tbl;
+
+			if (!(field->flags & TFW_STR_TRAILER))


We must skip headers with flags TFW_STR_HBH_HDR and header with hid TFW_HTTP_HDR_SERVER and I would prefer to skip TFW_STR_NOCCPY_HDR headers as well, even without modifying trailer header.

@krizhanovsky how do you think? I think that trailers are additional headers which should not be checked. Please answer here.

const-t · 2024-08-30T10:33:12Z

fw/cache.c

+			if (!first) {
+				trailers_trec = trec;
+				first = true;
+			}


Only suggestion.

Suggested change

if (!first) {

trailers_trec = trec;

first = true;

}

if (!trailers_trec)

trailers_trec = trec; //declare trailers_trec = NULL

fw/hpack.h

fw/http2.c

krizhanovsky

With #1902 (comment) I meant to remove the current logic of a trailer header movement to normal headers, even for HTTP/1, but with this PR I see a lot of new code, but not code removals.

The code complexity grows making the performance worse and maintenance harder.

Also now we still have the problem that Trailer: X-Token remains in headers.

Lastly, please resolve all comments which are actually resolved.

krizhanovsky · 2024-09-03T19:24:54Z

fw/cache.c

+	 * We don't move trailers to the end (after the body) for HTTP1
+	 * response, because when we build response from cache we assume
+	 * that we use content-length (fixed size), not chunked body.
+	 */


In #1902 (comment) I proposed to use only one chunk for HTTP/1. It's better to get as much as possible h1/h2 unified code: easier in testing, better performance and simpler code.

Also how the problem with trailer: X-Token header is solved in this PR then?

krizhanovsky · 2024-09-03T20:00:27Z

fw/http2.c

+	if (resp->trailers_len > 0) {
+		unsigned long acc = resp->mit.acc_len;
+		resp->mit.iter.skb = resp->msg.skb_head->prev;
+		resp->mit.iter.frag = skb_shinfo(resp->msg.skb_head->prev)->nr_frags - 1;


Too long line. No copyright updates.

Please enable git hooks with the coding style checks and automatic copyrights!

krizhanovsky · 2024-09-03T20:20:28Z

fw/http_msg.h

@@ -2,7 +2,7 @@
 *		Tempesta FW
 *
 * Copyright (C) 2014 NatSys Lab. ([email protected]).
- * Copyright (C) 2015-2023 Tempesta Technologies, Inc.
+ * Copyright (C) 2015-2024 Tempesta Technologies, Inc.


The only file with update copyright is the file, which wasn't updated :)

const-t

Needed small changes and discussion

fw/hpack.h

fw/http_frame.c

const-t · 2025-01-03T13:40:09Z

fw/ss_skb.c

 		if (r < 0) {
 			T_WARN("Can't delete len=%i from skb=%p\n", len, skb);
 			return r;
 		}
 		BUG_ON(r > len);

+		/* Check if the new skb was allocated and update next skb. */
+		next = skb->next != next ? skb->next : next;


Seems we can drop this condition as well as next variable.

const-t · 2025-01-03T13:47:54Z

fw/ss_skb.c

-		r = skb_fragment(skb_head, skb, ptr, -len, &it, &_);
+		to_delete = unlikely(abs(len) > PAGE_SIZE) ?
+			PAGE_SIZE : len;
+		next = skb->next;


Potential use-after-free here.

line 1049 we free it->skb inside __SS_SKB_CUTOFF_EMPTY().

line 1052 we assign freed it.skb to skb

return to line 1038 we use freed skb.

Looks like we should do kfree_skb(skb); instead of kfree_skb(it.skb);

const-t · 2025-01-03T13:58:08Z

fw/ss_skb.c

 		if (r < 0) {
 			T_WARN("Can't delete len=%i from skb=%p\n", len, skb);
 			return r;
 		}
 		BUG_ON(r > len);

+		/* Check if the new skb was allocated and update next skb. */
+		next = skb->next != next ? skb->next : next;
+		__SS_SKB_CUTOFF_EMPTY();


It's not a problem, it's just effort to minimize using of macro. I would ask you to rewrite __SS_SKB_CUTOFF_EMPTY() macro to static function. I'm sure that it will be inlined and will be the same as macro. Also could you rename __SS_SKB_CUTOFF_EMPTY() to something like __SS_SKB_FREE_EMPTY() to emphasize that this function frees skb.

fw/http_msg.c

fw/http_stream.c

fw/http_parser.c

const-t · 2025-01-03T15:54:54Z

fw/http_parser.c

@@ -2492,6 +2492,133 @@ __req_parse_transfer_encoding(TfwHttpMsg *hm, unsigned char *data, size_t len)
 	return __parse_transfer_encoding(hm, data, len, true, false);
 }

+static int
+__resp_parse_trailer(TfwHttpMsg *hm, unsigned char *data, size_t len)


Seems we allow duplicated Trailer header, though RFC doesn't contain direct prohibitions this looks like confusion and we must not allow duplicated Trailer header.

According RFC 9113: If the END_HEADERS flag is not set, this frame MUST be followed by another CONTINUATION frame. A receiver MUST treat the receipt of any other type of frame or a frame on a different stream as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. Tempesta FW should save all frames (if `ctx->cur_send_headers` is set) in special list and send them after sending headers frames and trailer frames.

Previously when we proxy http1 response to http2 client we send trailer headers as plain headers before data frames. But according RFC 9113 it is not correct - we should send trailer headers in separate header frame after the data frames.

1. `skb_fragment` can't delete more then PAGE_SIZE bytes, so if len > PAGE_SIZE we should not pass len directly to skb_fragment but calculate `len < PAGE_SIZE ? len : PAGE_SIZE` and go to the next loop iteration if len is to zero. 2. During deletion data from skb it can became empty. Previously we don't pay attention on this fact and delete such skbs later when we push it to socket write queue, but there is a problem when `tail != 0` in `ss_skb_cutoff_data`, because it.data == NULL for empty skb and `__ss_skb_cutoff` fails. (There is a BUG_ON(it.data == NULL) in `ss_skb_cutoff_data` to check it). To fix this problem we check if skb became empty and remove it immediatly).

According RFC 9113 trailers should be sent in additional header frame after headers and data frames. `Trailer` header should not be present in http2 response, so we mark 'Trailer' as `TFW_STR_TRAILER_HDR` to skip it when we convert http1 response to http2.

Previusly if Tempesta FW receives chunked response, we don't save transfer-encoding header in cache and don't restrore chunked body when we use this response to satisfy new request (instead of it we use plain body and content-length header). It is not correct when we have trailers in response, because trailers should be placed after chunked body. Now we save trailer, transfer-encoding and content length in cache, but restore it only if necessary. If we build http1 response we restore trailer and transfer-encoding from cache and use chunked body for http2 we use content-length and plain body.

- Remove 'Trailer' header for http1 and http2 requests/response. - Drop 'http1' requests/responses if trailer headers contain hop by hop headers.

We don't need to add content-length header for http2 responses at all. Also block response/request if 'upgrade' header is present in trailers. Closes #2209

According RFC 9110: Furthermore, intermediaries SHOULD remove or replace fields that are known to require removal before forwarding, whether or not they appear as a connection-option, after applying those fields' semantics. This includes but is not limited to: - Proxy-Connection (Appendix C.2.2 of [HTTP/1.1]) - Keep-Alive (Section 19.7.1 of [RFC2068]) - TE (Section 10.1.4) - Transfer-Encoding (Section 6.1 of [HTTP/1.1]) - Upgrade (Section 7.8)

There was an error when new skb is allocated, during `ss_skb_add_frag->__extend_pgfrags` call. We should update zero fragment of the new alocated skb not old one and also update it->skb and it->frag, when they passed to the `ss_skb_add_frag`. This patch fixes this problem.

kingluo requested review from const-t and EvgeniiMekhanik and removed request for const-t July 17, 2024 07:35