Fix HPACK dynamic table desync#2628
Conversation
f48b3f2 to
c7e24c8
Compare
|
@krizhanovsky @EvgeniiMekhanik the commit refactor: Move skb related functions to ss_skb.h is optional and can be dropped. It was introduced as part of the commit where I reworked trailers encoding, however we decided to not use HPACK dynamic table for trailers and I dropped that patch, but this commit with moving functions left in the PR. |
c7e24c8 to
9cd2132
Compare
| do { \ | ||
| if ((ctx->cur_##op##_headers \ | ||
| && (type != HTTP2_CONTINUATION && type != HTTP2_RST_STREAM)) \ | ||
| && ((type == HTTP2_HEADERS && !is_send) || \ |
There was a problem hiding this comment.
It seems that it is wrong. A HEADERS frame without the END_HEADERS flag set MUST be followed by a CONTINUATION frame for the same stream. 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.
It seems that HTTP2_RST_STREAM is also forbidden. So it should be
if ((ctx->cur_##op##_headers and type != HTTP2_CONTINUATION)
There was a problem hiding this comment.
I think it's better to check tcp window and do not prepare response. Or ignore tcp windo limit for headers frame
.
There was a problem hiding this comment.
Added fix for this as well.
There was a problem hiding this comment.
I think we should not prepare response if there is no enough TCP window or ignore tcp window during sending headers frame. Otherwise we need make incorrect change in stream state processing. @krizhanovsky what do you think?
|
I also found two problems in
|
723a84e to
2912200
Compare
2912200 to
f9687bb
Compare
I've updated PR. Added fixes for #2640. But I didn't implement Actually this patch allows to call |
| */ | ||
| if (tbl->window != new_size && (likely(!tbl->wnd_changed) | ||
| || unlikely(!tbl->window) || new_size < tbl->window)) | ||
| if (tbl->window != requested_size && (likely(!tbl->wnd_changed) |
There was a problem hiding this comment.
I read RFC again and look for nghtt2 realization - in some cases we should apply and send two hpack dynamic changes!
For example
4096 - > 1 -> 100 we should send 1 and 100 (minimum and final)
4096 -> 1 -> 0 -> 2 -> 100 -> 3 we should send 0 and 3 (minimum and final)
There was a problem hiding this comment.
Good catch, another one bug. Tempesta must be able to send min and max size. RFC cite:
Multiple updates to the maximum table size can occur between the
transmission of two header blocks. In the case that this size is
changed more than once in this interval, the smallest maximum table
size that occurs in that interval MUST be signaled in a dynamic table
size update. The final maximum size is always signaled, resulting in
at most two dynamic table size updates. This ensures that the
decoder is able to perform eviction based on reductions in dynamic
table size
However maybe it makes sense to implement it in other PR, to keep this PR small.
There was a problem hiding this comment.
Yes also look nghttp2_hd_deflate_hd_bufs in nghttp2
if (deflater->ctx.hd_table_bufsize_max > min_hd_table_bufsize_max) {
rv = emit_table_size(bufs, min_hd_table_bufsize_max);
if (rv != 0) {
goto fail;
}
}
rv = emit_table_size(bufs, deflater->ctx.hd_table_bufsize_max);
|
There are some questions about patch:
|
| } else if (res == STREAM_FSM_RES_TERM_STREAM) { \ | ||
| WARN_ON_ONCE(hdr->stream_id != ctx->cur_stream->id); \ | ||
| return tfw_h2_current_stream_send_rst((ctx), err); \ | ||
| return tfw_h2_send_rst_stream(ctx, ctx->cur_stream->id,\ |
There was a problem hiding this comment.
May we can implement tfw_h2_on_tcp_entail_rst (like tfw_h2_on_tcp_entail_ack). to move stream to closed queue?
There was a problem hiding this comment.
Currently it will be moved to closed queue during making data frames (because of error in tfw_h2_insert_frame_header->tfw_h2_stream_fsm_ignore_err but it is not clear (as for me)
There was a problem hiding this comment.
Currently it will be moved to closed queue during making data frames (because of error in tfw_h2_insert_frame_header->tfw_h2_stream_fsm_ignore_err but it is not clear (as for me)
This is correct for requests for that we will prepare response, however if response will not be prepared we will not add stream to closed and don't clean before connection closing. I will fix this.
f9687bb to
0edde4e
Compare
Advertise the selected dynamic table size if the client's table size is not equal to Tempesta FW dynamic table size. Details: when Tempesta receives SETTINGS frame and SETTINGS_HEADER_TABLE_SIZE in this frame is greater than default Tempesta's 4096, Tempesta advertise the size that it will be used. Before this patch Tempesta ignored it when Tempesta's table size is equal to default 4096 and size advertised by the client is greater than 4096. In other cases Tempesta did send updates. For reference see: RFC7541 Section 4.2
This patch fixes two issues with HPACK desync. The first one appears when we encode trailer headers at the moment of encoding regular headers For example: Tempesta encoded headers and trailers and proceed to sending the body. During sending the body scheduler few times preemt the current stream to send headers for another stream, therefore HPACK dynamic table contains the new headers and indexes used for trailers are invalid. To fix this we decided to not use HPACK dynamic table for trailers, it is a good trade-off between complexity and efficiency. The reason of the second is not sending new size of the dynamic table in trailer HEADERS frame. When client advertised the new size of the dynamic table Tempesta must respond with selected size in the first HEADERS it doesn't matter whether they are trailers or regular headers. Small refactoring to improve readability
By the standart during sending headers block any others frames are forbidden except CONTINUATION frame
Before this patch Tempesta stopped to send headers block when RST_STREAM frame was received or sent, that broke HPACK state. In this patch we sending all encoded headers to maintain synchronized HPACK dynamic table.
The case when Tempesta receives obviously broken HEADERS or PRIORITY frame Tempesta treats it as suspicios and do disconnect instead of handling this case with RST_STREAM. It looks that it doesn't makes sense to continue service this connection. Also this removes the attack vector for RST_STREAM flood. RFC 9113 5.4.1. Connection Error Handling: An endpoint can end a connection at any time. In particular, an endpoint MAY choose to treat a stream error as a connection error.
Do this to ensure that the function calculating frame size accounts for the dynamic table size, not just the headers, and does not produce CONTINUATION frame if the data fits within a single HEADERS frame. The main reason to fix this: It seems firefox has a bug because of that it can't proccess CONTINUATION frame for closed stream and resets connection breaking page loading.
The function must have only one responsibility insert frame headers, however before this patch it also sent data. In this patch we extract data sending logic into dedicated state to make code more understandable and explicit
This patch enables splitting HEADERS and CONTINUATION frames according to the maximum frame length and sends the resulting frame fragments as the sending window becomes available. Length of prepared frame stored in `TfwHttpXmit` struct. This avoids generating a large number of small CONTINUATION frames, which may be rejected by some implementations, such as nghttp.
This is a similar optimization to the one used in the previous framing implementation. However, this version enforces the minimum sending budget only for the TCP send window, not for the HTTP/2 flow-control window. This allows to send small DATA frame and proceed to the next stream if TCP window is available. In addition, this patch no longer stops re-scheduling when a stream's flow-control window is exhausted. Instead, it continues searching for the next stream with available window capacity.
According to the HTTP/2 specification, when two or more `SETTINGS_HEADER_TABLE_SIZE` values are received between header blocks, the receiver must send at most two dynamic table updates. These updates must include the minimum received and applied value and the currently applied value. The minimum applied value is needed for the sender of the setting `SETTINGS_HEADER_TABLE_SIZE` to perform the same eviction in the decoder’s dynamic table.
0edde4e to
b462bbf
Compare
This patch fixes two issues related to SETTINGS handling. The first is a kernel panic that occurs when applying a new `SETTINGS_INITIAL_WINDOW_SIZE`. When Tempesta receives a new `SETTINGS_INITIAL_WINDOW_SIZE`, it does not apply the setting immediately but postpones it until data is added to the write queue. If the new `SETTINGS_INITIAL_WINDOW_SIZE` is applied while responding to the client, and a stream has been marked as blocked (`stream->xmit.is_blocked = 1`) due to an exhausted HTTP/2 flow-control window while it still has pending HEADERS frames to send, a kernel panic occurs. When applying SETTINGS_INITIAL_WINDOW_SIZE, we update the window size of blocked streams. However, the stream marked as `is_blocked` are not yet in the blocked streams queue because transmission of the stream has not completed. See stack trace for details. To fix this, settings are now applied immediately upon receipt, and a new frame-sending function `__tfw_h2_send_frame_local()` is introduced. Unlike the existing implementation, this function bypasses the work queue and queues the new frame directly into the connection's (not socket's) write queue, or into the stream's postponed queue if a HEADERS block is currently being transmitted. This approach has several advantages: 1. It avoids unnecessary use of the work queue, since the code already runs under the lock and the frame is prepared on the local CPU. 2. It applies the new settings as early as possible, prioritizing transmission of the SETTINGS ACK before DATA frames. This behavior is consistent with RFC 9113 Section 6.5.3, which states: "A recipient of a SETTINGS frame in which the ACK flag is not set MUST apply the updated settings as soon as possible upon receipt ... Once all values have been processed, the recipient MUST immediately emit a SETTINGS frame with the ACK flag set." 3. It enables future optimizations, such as appending new frames to an existing skb in the queue instead of allocating a new one. part of stack trace: tfw_h2_stream_sched_insert_active+0xd5/0xe0 tfw_h2_sched_activate_stream+0x69/0x80 [tempesta_fw] tfw_h2_apply_new_settings+0x1f3/0x260 [tempesta_fw] ss_skb_tcp_entail+0x43/0x3c0 [tempesta_fw] ss_skb_tcp_entail_list+0x124/0x560 [tempesta_fw] tfw_h2_stream_send_postponed+0x28/0xc0 [tempesta_fw] tfw_h2_make_frames+0x604/0xac0 [tempesta_fw] tfw_connection_fill_sk_write_queue+0x119/0x260 [tempesta_fw] tcp_push_pending_frames+0x33/0x160 The second issue is minor but still fixed by this patch. When two or more SETTINGS frames are received with the same parameter but different values, each value must be applied immediately, and a SETTINGS ACK must be sent for each update. Before this fix, only the last value was applied, overriding previous ones and causing some updates to be lost. For example, if Tempesta receives SETTINGS_HEADER_TABLE_SIZE=0 and then SETTINGS_HEADER_TABLE_SIZE=4096, it must apply both values. The sender may intend to clear the table before applying the new settings, sending 0 table size. Previously, Tempesta applied only the last value (4096), ignoring previous update.
`tfw_hpack_enc_tbl_write_sz()` is a part of framing, it is not a part of hpack implementation
b462bbf to
8c5bd58
Compare
No description provided.