From: Tomas Krizek <tomas.krizek@nic.cz>
Date: Wed, 18 Aug 2021 13:18:27 +0000 (+0200)
Subject: doh2: ensure memory from unsent streams is freed
X-Git-Tag: v5.4.1~1^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=da6bbccb04619fa2f711e329110edc5446e265ce;p=thirdparty%2Fknot-resolver.git

doh2: ensure memory from unsent streams is freed

The nghttp2 on_stream_close callback is only called for streams that are
properly closed. If we need to tear down the HTTP connection due to any
reason (e.g. IO error in underlying layer), some streams may not be
propely closed.

Due to HTTP/2 flow control, we may also wait indefinitely for the data
to be written. This can also cause the stream to never be properly
closed.

To handle these cases, a reference of allocated data is kept and we
ensure everything is freed once we're closing the http session.
---

diff --git a/NEWS b/NEWS
index 91a385e10..0114838e6 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,7 @@ Bugfixes
 --------
 - fix build without doh2 support after 5.4.0 (!1197)
 - fix policy.DEBUG* logging and -V/--version after 5.4.0 (!1199)
+- doh2: ensure memory from unsent streams is freed (!1202)
 
 
 Knot Resolver 5.4.0 (2021-07-29)
diff --git a/daemon/http.c b/daemon/http.c
index 4a70de99f..44bfdce8e 100644
--- a/daemon/http.c
+++ b/daemon/http.c
@@ -521,6 +521,12 @@ static void on_pkt_write(struct http_data *data, int status)
 	free(data);
 }
 
+static int stream_write_data_free_err(trie_val_t *val, void *null)
+{
+	on_pkt_write(*val, kr_error(EIO));
+	return 0;
+}
+
 /*
  * Cleanup for closed streams.
  *
@@ -538,6 +544,7 @@ static int on_stream_close_callback(nghttp2_session *h2, int32_t stream_id,
 	if (ctx->incomplete_stream == stream_id)
 		http_cleanup_stream(ctx);
 
+	trie_del(ctx->stream_write_data, (char *)&stream_id, sizeof(stream_id), NULL);
 	data = nghttp2_session_get_stream_user_data(h2, stream_id);
 	if (data)
 		on_pkt_write(data, error_code == 0 ? 0 : kr_error(EIO));
@@ -578,6 +585,7 @@ struct http_ctx* http_new(struct session *session, http_send_callback send_cb)
 	ctx->send_cb = send_cb;
 	ctx->session = session;
 	queue_init(ctx->streams);
+	ctx->stream_write_data = trie_create(NULL);
 	ctx->incomplete_stream = -1;
 	ctx->submitted = 0;
 	ctx->current_method = HTTP_METHOD_NONE;
@@ -670,9 +678,10 @@ static ssize_t read_callback(nghttp2_session *h2, int32_t stream_id, uint8_t *bu
  *
  * Data isn't guaranteed to be sent immediately due to underlying HTTP/2 flow control.
  */
-static int http_send_response(nghttp2_session *h2, int32_t stream_id,
+static int http_send_response(struct http_ctx *ctx, int32_t stream_id,
 			      nghttp2_data_provider *prov)
 {
+	nghttp2_session *h2 = ctx->h2;
 	struct http_data *data = (struct http_data*)prov->source.ptr;
 	int ret;
 	const char *directive_max_age = "max-age=";
@@ -707,6 +716,16 @@ static int http_send_response(nghttp2_session *h2, int32_t stream_id,
 		return kr_error(EIO);
 	}
 
+	/* Keep reference to data, since we need to free it later on.
+	 * Due to HTTP/2 flow control, this stream data may be sent at a later point, or not at all.
+	 */
+	trie_val_t *stream_data_p = trie_get_ins(ctx->stream_write_data, (char *)&stream_id, sizeof(stream_id));
+	if (kr_fails_assert(stream_data_p)) {
+		kr_log_debug(DOH, "[%p] failed to insert to stream_write_data\n", (void *)h2);
+		free(data);
+		return kr_error(EIO);
+	}
+	*stream_data_p = data;
 	ret = nghttp2_session_send(h2);
 	if(ret < 0) {
 		kr_log_debug(DOH, "[%p] nghttp2_session_send failed: %s\n", (void *)h2, nghttp2_strerror(ret));
@@ -731,7 +750,7 @@ static int http_send_response(nghttp2_session *h2, int32_t stream_id,
  * musn't be!) called, since such errors are handled in an upper layer - in
  * qr_task_step() in daemon/worker.
  */
-static int http_write_pkt(nghttp2_session *h2, knot_pkt_t *pkt, int32_t stream_id,
+static int http_write_pkt(struct http_ctx *ctx, knot_pkt_t *pkt, int32_t stream_id,
 			  uv_write_t *req, uv_write_cb on_write)
 {
 	struct http_data *data;
@@ -752,7 +771,7 @@ static int http_write_pkt(nghttp2_session *h2, knot_pkt_t *pkt, int32_t stream_i
 	prov.source.ptr = data;
 	prov.read_callback = read_callback;
 
-	return http_send_response(h2, stream_id, &prov);
+	return http_send_response(ctx, stream_id, &prov);
 }
 
 /*
@@ -779,7 +798,7 @@ int http_write(uv_write_t *req, uv_handle_t *handle, knot_pkt_t *pkt, int32_t st
 	if (!ctx || !ctx->h2)
 		return kr_error(EINVAL);
 
-	ret = http_write_pkt(ctx->h2, pkt, stream_id, req, on_write);
+	ret = http_write_pkt(ctx, pkt, stream_id, req, on_write);
 	if (ret < 0)
 		return ret;
 
@@ -807,6 +826,9 @@ void http_free(struct http_ctx *ctx)
 		queue_pop(ctx->streams);
 	}
 
+	trie_apply(ctx->stream_write_data, stream_write_data_free_err, NULL);
+	trie_free(ctx->stream_write_data);
+
 	http_cleanup_stream(ctx);
 	queue_deinit(ctx->streams);
 	nghttp2_session_del(ctx->h2);
diff --git a/daemon/http.h b/daemon/http.h
index e76d211f2..7372fd899 100644
--- a/daemon/http.h
+++ b/daemon/http.h
@@ -42,6 +42,7 @@ struct http_ctx {
 	http_send_callback send_cb;
 	struct session *session;
 	queue_http_stream streams;  /* Streams present in the wire buffer. */
+	trie_t *stream_write_data;  /* Dictionary of stream data that needs to be freed after write. */
 	int32_t incomplete_stream;
 	ssize_t submitted;
 	http_method_t current_method;
diff --git a/daemon/session.c b/daemon/session.c
index 668295ca0..c1e23b929 100644
--- a/daemon/session.c
+++ b/daemon/session.c
@@ -83,14 +83,14 @@ void session_clear(struct session *session)
 	if (session->handle && session->handle->type == UV_TCP) {
 		free(session->wire_buf);
 	}
+#if ENABLE_DOH2
+	http_free(session->http_ctx);
+#endif
 	trie_clear(session->tasks);
 	trie_free(session->tasks);
 	queue_deinit(session->waiting);
 	tls_free(session->tls_ctx);
 	tls_client_ctx_free(session->tls_client_ctx);
-#if ENABLE_DOH2
-	http_free(session->http_ctx);
-#endif
 	memset(session, 0, sizeof(*session));
 }