Fix HPACK dynamic table desync

[const-t]

No description provided.

[const-t]

@krizhanovsky @EvgeniiMekhanik This is alternative solution that lets to choose another stream on the next scheduling cycle, because not encoding header if they can't be sent due to too small window (snd_wnd <= FRAME_HEADER_SIZE + TLS_MAX_OVERHEAD window too small for any stream not only for the current), but has worse latency because of postponing encoding. And may postpone already framed stream, that leads to smaller HEADERS frame than possible and forces to use CONTINUATION frame.

From my point of view, setting of ctx->cur_send_headers directly is not consistent, but maybe you guys have another vision.

diff --git a/fw/http_frame.c b/fw/http_frame.c
index 8e9b29a0c..968d1c360 100644
--- a/fw/http_frame.c
+++ b/fw/http_frame.c
@@ -2161,6 +2161,13 @@ do {									\
 	T_FSM_EXIT();							\
 } while(0)

+#define CALC_MAX_MIN_FRAME_LENGTH(len, out_max, out_min)		\
+do {									\
+	(out_max) = min(TLS_MAX_PAYLOAD_SIZE, *snd_wnd - TLS_MAX_OVERHEAD); \
+	(out_max) -= FRAME_HEADER_SIZE;					\
+	(out_min) = min(min_to_send, (unsigned int)(len));		\
+} while (0)
+
 #define CALC_FRAME_LENGTH_AND_SET_FRAME_TYPE(type, len)			\
 do {									\
 	unsigned int max_len;						\
@@ -2170,9 +2177,7 @@ do {									\
 		*stop = true;						\
 		T_FSM_EXIT();						\
 	}								\
-	max_len = min(TLS_MAX_PAYLOAD_SIZE, *snd_wnd - TLS_MAX_OVERHEAD); \
-	max_len -= FRAME_HEADER_SIZE;					\
-	min_len = min(min_to_send, (unsigned int)len);			\
+	CALC_MAX_MIN_FRAME_LENGTH(len, max_len, min_len);		\
 	frame_length = tfw_h2_calc_frame_length(ctx, stream, type, len,	\
 						max_len); 		\
 	/*								\
@@ -2190,9 +2195,16 @@ do {									\
 	T_FSM_JMP(state);						\
 } while(0)

+#define FRAME_XMIT_FSM_SET_NEXT_STATE(state)				\
+	__fsm_const_state = state
+
 	T_FSM_START(stream->xmit.state) {

 	T_FSM_STATE(HTTP2_ENCODE_HEADERS) {
+		if (*snd_wnd <= FRAME_HEADER_SIZE + TLS_MAX_OVERHEAD) {
+			*stop = true;
+			T_FSM_EXIT();
+		}
 		r = tfw_h2_stream_xmit_prepare_resp(stream);
 		fallthrough;
 	}
@@ -2210,8 +2222,14 @@ do {									\
 	}

 	T_FSM_STATE(HTTP2_MAKE_HEADERS_FRAMES) {
-		CALC_FRAME_LENGTH_AND_SET_FRAME_TYPE(HTTP2_HEADERS,
-						     stream->xmit.h_len);
+		unsigned int min_len, max_len;
+
+		CALC_MAX_MIN_FRAME_LENGTH(stream->xmit.h_len, max_len, min_len);
+		frame_length = tfw_h2_calc_frame_length(ctx, stream,
+							HTTP2_HEADERS,
+							stream->xmit.h_len,
+							max_len);
+
 		if (unlikely(ctx->hpack.enc_tbl.wnd_changed)) {
 			r = tfw_hpack_enc_tbl_write_sz(&ctx->hpack.enc_tbl,
 						       stream);
@@ -2229,6 +2247,12 @@ do {									\
 			return r;
 		}

+		if (frame_length < min_len) {
+			FRAME_XMIT_FSM_SET_NEXT_STATE(HTTP2_SEND_FRAMES);
+			*stop = true;
+			T_FSM_EXIT();
+		}
+
 		FRAME_XMIT_FSM_NEXT(frame_length, HTTP2_SEND_FRAMES);
 	}

[const-t]

@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.

[EvgeniiMekhanik]

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)

[EvgeniiMekhanik]

I think it's better to check tcp window and do not prepare response. Or ignore tcp windo limit for headers frame
.

[const-t]

Added fix for this as well.

[EvgeniiMekhanik]

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?

[EvgeniiMekhanik]

I also found two problems in
f (frame_length < min_len)
ADJUST_BLOCKED_STREAMS_AND_EXIT(min_len, type);
optimization:

• we stop making frames if stream http2 window is exceeded. But if we have other streams to make frames we should do it.
• we stop making frames if http2 connection window is exceeded. But if we have other streams with headers frames
  we should continue make frames for them.

[const-t]

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've updated PR. Added fixes for #2640. But I didn't implement I think we should not prepare response if there is no enough TCP window. From my point of view, if we reached framing function we can prepare headers even if tcp window is not enough, to have prepared headers on the next shot of calling sending function and don't call stream scheduler one more time. However I attached the version where we don't prepare response if there is no enough TCP window in the comment to see the difference.

Actually this patch allows to call tfw_h2_stream_fsm_ignore_err() multiple times for HEADERS frame in the state HTTP2_STREAM_REM_HALF_CLOSED only during sending that doesn't looks like issue, because we only assign ctx->cur_send_headers in this case and not change state of the stream, frames received from the client will not affect this change only sending side.

[EvgeniiMekhanik]

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)

[const-t]

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.

[EvgeniiMekhanik]

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);

[EvgeniiMekhanik]

There are some questions about patch:

1. If we can't send any other frames for other streams after sending HEADERS FRAME without END_STREAM flag for current stream may be it have sense to don't calculate snd_wnd during sending headers at all? In this case we don't need some commits from this patch. (UPDATE. I check h20 implementation they don't send all headers to socket immediately< also we rely on window when we account client mem, so we still should calculate snd_wnd during sending headers, but may be not do it so accuracy).
2. We should check how another implementations works with RST STREAM receiving. It seems that now be make some changes not according RFC. It looks like we are trying to simply hide the current problem rather than solve it globally. (I mean that we continue to send headers frames after receiving RST STREAM and more over we don't process it now at all!)

[EvgeniiMekhanik]

May we can implement tfw_h2_on_tcp_entail_rst (like tfw_h2_on_tcp_entail_ack). to move stream to closed queue?

[EvgeniiMekhanik]

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)

[const-t]

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.