Fix invalid stream processing #1887

EvgeniiMekhanik · 2023-06-01T06:52:02Z

We should split stream processing into two
parts: in first part we pass HTTP2_HEADERS
type and HTTP2_F_END_HEADERS flag to
STREAM_SEND_PROCESS and later in xmit
callback we should pass HTTP2_F_END_STREAM
flag and appropriate type when we finish
send all data to corrsponding stream.

const-t

Part related to stream processing looks good.

const-t · 2023-06-19T16:37:36Z

fw/http_frame.c

-int
-tfw_h2_stream_process(TfwH2Ctx *ctx, TfwStream *stream, unsigned char type)
+TfwStreamFsmRes
+tfw_h2_stream_process(TfwH2Ctx *ctx, TfwStream *stream,


Seems tfw_h2_stream_process() must be called only if stream->xmit.b_len == 0 or stream->xmit.h_len == 0 otherwise it looks dangerous. Now it's ok, but maybe add BUG?

const-t · 2023-06-19T16:58:44Z

fw/sock_clnt.c

+#define H2_PROCESS_STREAM(h2, stream, type)				\
+	r = tfw_h2_stream_process(h2, stream, type);			\
+	if (unlikely(r != STREAM_FSM_RES_OK)) {				\
+		T_WARN("Failed tp process stream %d", (int)r);		\


krizhanovsky

LGTM with minor cleanups

krizhanovsky · 2023-06-23T11:09:15Z

fw/main.c

@@ -22,6 +22,7 @@
 #include <asm/fpu/api.h>


Please update the copyright year

krizhanovsky · 2023-06-23T11:15:11Z

fw/main.c

@@ -409,6 +471,14 @@ static struct ctl_table tfw_sysctl_tbl[] = {
 		.mode		= 0644,
 		.proc_handler	= tfw_ctlfn_state_io,
 	},
+#ifdef DBG_ERRINJ
+	{
+		.procname	= "errinj",


In short, there is some runtime API and some C API and we do not have any examples of the usage at the moment. Could you please add an example and a short HowTo to the wiki? Maybe extend https://github.com/tempesta-tech/tempesta/wiki/Debugging-and-troubleshooting or create a new page under Contributing.

krizhanovsky · 2023-06-23T11:19:12Z

lib/errinj.c

+ * on top on native Linux socket buffers. The helpers provide common and
+ * convenient wrappers for skb processing.
+ *
+ * Copyright (C) 2015-2023 Tempesta Technologies, Inc.


Usually for new files we use only current year, so there should be just 2023. If you see 2015-2023, then it's quite an old file ;)

lib/errinj.h

krizhanovsky · 2023-06-23T11:22:09Z

lib/errinj.c

+ *
+ * Application protocol handler layers must implement zero data copy logic
+ * on top on native Linux socket buffers. The helpers provide common and
+ * convenient wrappers for skb processing.


I didn't get the comment - why error injection file comments something about skb? A dirty copy & paste? BTW it'd be good to write some description about the error injection design, maybe some usage example

krizhanovsky · 2023-06-23T11:23:09Z

lib/errinj.c

+        ERRINJ_LIST(ERRINJ_MEMBER)
+};
+EXPORT_SYMBOL(errinjs);
+


extra empty line

krizhanovsky · 2023-06-23T11:23:37Z

lib/errinj.c

+
+#include "errinj.h"
+
+#define ERRINJ_MEMBER(n, t, s) { /* .name = */ #n, /* .type = */ t, /* .state = */ s },


too long line

krizhanovsky · 2023-06-23T11:25:57Z

lib/errinj.h

+	union {
+		/** bool parameter */
+		bool bparam;
+		/** integer parameter */


Usually we write a single comment on top of a structure describing all the members - it's not good or bad, but it'd be good to adjust for code consistency

krizhanovsky · 2023-06-23T11:26:50Z

lib/errinj.h

+#  define ERROR_INJECT(ID, CODE) \
+	do { \
+		if (errinj_get(ID, ERRINJ_BOOL)->bparam) \
+			CODE; \


Let's place all \ aligned as for ERROR_INJECT_COUNTDOWN and around the code

Implement error injection to test difficult cases, which can't be simulated by usual python tests.

We should split stream processing into two parts: in first part we pass `HTTP2_HEADERS` type and `HTTP2_F_END_HEADERS` flag to `STREAM_SEND_PROCESS` and later in `xmit` callback we should pass `HTTP2_F_END_STREAM` flag and appropriate type when we finish send all data for corresponding stream. Also rename `tfw_h2_stream_id_unlink` to `tfw_h2_stream_unlink_from_req`

const-t

LGTM

const-t · 2023-06-28T08:20:02Z

fw/http_frame.c

-		    || (max_streams == ctx->streams_num
-			&& closed_streams->num))


IIUC this condition need to not delete streams if we have maximum streams and some streams in closed queue. These streams will be deleted on closing connection. Is there any reason to remove it?

Yes, fixed during discussion. Add comment to #1346 to don't forget to change this condition later

const-t · 2023-06-28T08:24:37Z

fw/http_frame.h

@@ -228,6 +228,7 @@ int tfw_h2_context_init(TfwH2Ctx *ctx);
 void tfw_h2_context_clear(TfwH2Ctx *ctx);
 int tfw_h2_frame_process(TfwConn *c, struct sk_buff *skb);
 void tfw_h2_conn_streams_cleanup(TfwH2Ctx *ctx);
+TfwStream *tfw_h2_find_stream_if_not_closed(TfwH2Ctx *ctx, unsigned int id);


It's not mandatory, but I suggest to renaming tfw_h2_find_stream_if_not_closed to tfw_h2_find_stream_not_closed or to tfw_h2_find_not_closed_stream

Tempesta uses delayed streams deletion (closed streams add to special queue and remove later, when there count become greater than appropriate constant). We should not work with such streams except cases, when we process HTTP2_PRIORITY, HTTP2_WINDOW_UPDATE or HTTP2_RST_STREAM frames (according to RFC 9113), for this cases we should ignore such frames. Also remove strange condition from function `tfw_h2_closed_streams_shrink` (we can't delete streams from closed queue if there count in this queue is equal to zero). Closes #1800

dmpetroff

I've only reviewed part related to tfw_h2_stream_id_verify, and it looks similar to what I did in #1913.

EvgeniiMekhanik marked this pull request as draft June 1, 2023 06:53

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch 5 times, most recently from 5aeebde to c7f3e00 Compare June 1, 2023 10:04

EvgeniiMekhanik requested review from krizhanovsky and const-t June 1, 2023 10:12

EvgeniiMekhanik marked this pull request as ready for review June 1, 2023 10:12

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch 2 times, most recently from 2f51a38 to 1021f5f Compare June 8, 2023 08:31

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch from 1021f5f to 6e9c512 Compare June 8, 2023 15:31

const-t approved these changes Jun 20, 2023

View reviewed changes

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch 4 times, most recently from fa7df28 to f62e4c2 Compare June 22, 2023 12:21

krizhanovsky approved these changes Jun 23, 2023

View reviewed changes

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch from 6bb718c to f2ebd63 Compare June 26, 2023 11:52

EvgeniiMekhanik marked this pull request as draft June 26, 2023 11:53

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch 2 times, most recently from e2b9ec1 to d097b03 Compare June 26, 2023 14:13

EvgeniiMekhanik marked this pull request as ready for review June 26, 2023 14:13

const-t mentioned this pull request Jun 26, 2023

Validate stream_id from HEADERS frame #1913

Closed

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch 3 times, most recently from f20dc6e to 1f866aa Compare June 27, 2023 15:28

EvgeniiMekhanik added 3 commits June 27, 2023 18:29

Implement base error injection

b364df2

Implement error injection to test difficult cases, which can't be simulated by usual python tests.

Fix according review

8c6e64b

Change tfw_h2_stream_fsm according to RFC9113

eeb794d

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch from 1f866aa to 09891fb Compare June 27, 2023 15:30

EvgeniiMekhanik requested a review from const-t June 28, 2023 07:31

const-t approved these changes Jun 28, 2023

View reviewed changes

EvgeniiMekhanik force-pushed the MekhanikEvgenii/Fix-incorrect-stream-processing branch from 09891fb to a88e643 Compare June 28, 2023 08:59

krizhanovsky requested a review from dmpetroff June 28, 2023 09:46

dmpetroff approved these changes Jun 28, 2023

View reviewed changes

EvgeniiMekhanik merged commit 8a13b3f into master Jun 28, 2023

EvgeniiMekhanik deleted the MekhanikEvgenii/Fix-incorrect-stream-processing branch June 28, 2023 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid stream processing #1887

Fix invalid stream processing #1887

EvgeniiMekhanik commented Jun 1, 2023

const-t left a comment

const-t Jun 19, 2023 •

edited

Loading

EvgeniiMekhanik Jun 20, 2023

const-t Jun 19, 2023

EvgeniiMekhanik Jun 20, 2023

krizhanovsky left a comment

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

krizhanovsky Jun 23, 2023

EvgeniiMekhanik Jun 26, 2023

const-t left a comment

const-t Jun 28, 2023

EvgeniiMekhanik Jun 28, 2023

const-t Jun 28, 2023

EvgeniiMekhanik Jun 28, 2023

dmpetroff left a comment


		#include "errinj.h"

		#define ERRINJ_MEMBER(n, t, s) { /* .name = / #n, / .type = / t, / .state = */ s },

Fix invalid stream processing #1887

Fix invalid stream processing #1887

Conversation

EvgeniiMekhanik commented Jun 1, 2023

const-t left a comment

Choose a reason for hiding this comment

const-t Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

const-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmpetroff left a comment

Choose a reason for hiding this comment

const-t Jun 19, 2023 •

edited

Loading