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 91a385e10ab8d79ced1ebb95b967dcf30daa86de..0114838e61b0da49286c751c6cbeaa9274ba009c 100644 (file)
--- 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 4a70de99f383a6194cca7f01dca2502eb9e70c9d..44bfdce8e65ee0a57413316a5983810c24275e62 100644 (file)
--- 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 e76d211f2d612cfe4e3056ce87049af44790d307..7372fd8996d114586466bc4492e8c32d1e3b8f09 100644 (file)
--- 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 668295ca0ea24284fd9f2aa6567219ad3962bd8e..c1e23b929aadeec99862cb32b54a675b61c0da3f 100644 (file)
--- 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));
 }