daemon/http: copy headers to streams instead of ownership transfer

diff --git a/daemon/bindings/net_tlssrv.rst b/daemon/bindings/net_tlssrv.rst
index 519a0b5690b15f5ff7ee80f489ce29eb5e8ce66d..f496cd70ee82f872d598b072793db1beb076ae4d 100644 (file)
--- a/daemon/bindings/net_tlssrv.rst
+++ b/daemon/bindings/net_tlssrv.rst
@@ -71,7 +71,7 @@ additional considerations for TLS 1.2 required by HTTP/2 are not implemented
 HTTP status codes
 """""""""""""""""
 
-As specified by :rfc:`8484`, the resolver responds with status **200 OK** whenever 
+As specified by :rfc:`8484`, the resolver responds with status **200 OK** whenever
 it can produce a valid DNS reply for a given query, even in cases where the DNS
 ``rcode`` indicates an error (like ``NXDOMAIN``, ``SERVFAIL``, etc.).
 
@@ -80,7 +80,7 @@ the following status codes:
 
  * **400 Bad Request** for a generally malformed query, like one not containing
    a valid DNS packet
- * **404 Not Found** when an incorrect HTTP endpoint is queried - the only 
+ * **404 Not Found** when an incorrect HTTP endpoint is queried - the only
    supported ones are ``/dns-query`` and ``/doh``
  * **413 Payload Too Large** when the DNS query exceeds its maximum size
  * **415 Unsupported Media Type** when the query's ``Content-Type`` header

diff --git a/daemon/http.c b/daemon/http.c
index adb79eeb9ff65628b6b9bcf33d202430d11d89a2..2e8aa2f00c003564179e7148b6f6033ee726550f 100644 (file)
--- a/daemon/http.c
+++ b/daemon/http.c
@@ -65,6 +65,13 @@ static int http_send_response(struct http_ctx *ctx, int32_t stream_id,
 static int http_send_response_rst_stream(struct http_ctx *ctx, int32_t stream_id,
                              nghttp2_data_provider *prov, enum http_status status);
 
+/** Checks if `status` has the correct `category`.
+ * E.g. status 200 has category 2, status 404 has category 4, 501 has category 5 etc. */
+static inline bool http_status_has_category(enum http_status status, int category)
+{
+       return status / 100 == category;
+}
+
 /*
  * Write HTTP/2 protocol data to underlying transport layer.
  */
@@ -232,6 +239,23 @@ static int check_uri(const char* uri_path)
        return 0;
 }
 
+static kr_http_header_array_t *headers_dup(kr_http_header_array_t *src)
+{
+       kr_http_header_array_t *dst = malloc(sizeof(kr_http_header_array_t));
+       kr_require(dst);
+       array_init(*dst);
+       for (size_t i = 0; i < src->len; i++) {
+               struct kr_http_header_array_entry *src_entry = &src->at[i];
+               struct kr_http_header_array_entry dst_entry = {
+                       .name = strdup(src_entry->name),
+                       .value = strdup(src_entry->value)
+               };
+               array_push(*dst, dst_entry);
+       }
+
+       return dst;
+}
+
 /*
  * Process a query from URI path if there's base64url encoded dns variable.
  */
@@ -269,7 +293,7 @@ static int process_uri_path(struct http_ctx *ctx, const char* path, int32_t stre
 
        struct http_stream stream = {
                .id = stream_id,
-               .headers = ctx->headers
+               .headers = headers_dup(ctx->headers)
        };
        queue_push(ctx->streams, stream);
        return 0;
@@ -476,7 +500,7 @@ static int data_chunk_recv_callback(nghttp2_session *h2, uint8_t flags, int32_t
                ctx->buf_pos = sizeof(uint16_t);  /* Reserve 2B for dnsmsg len. */
                struct http_stream stream = {
                        .id = stream_id,
-                       .headers = ctx->headers
+                       .headers = headers_dup(ctx->headers)
                };
                queue_push(ctx->streams, stream);
        }
@@ -491,7 +515,8 @@ static int submit_to_wirebuffer(struct http_ctx *ctx)
        int ret = -1;
        ssize_t len;
 
-       /* Transfer ownership to stream (waiting in wirebuffer) */
+       /* Free http_ctx's headers - by now the stream has obtained its own
+        * copy of the headers which it can operate on. */
        /* FIXME: technically, transferring memory ownership should happen
         * along with queue_push(ctx->streams) to avoid confusion of who owns
         * what and when. Pushing to queue should be done AFTER we successfully
@@ -501,7 +526,16 @@ static int submit_to_wirebuffer(struct http_ctx *ctx)
         *
         * For now, we assume memory is transferred even on error and the
         * headers themselves get cleaned up during http_free() which is
-        * triggered after the error when session is closed.  */
+        * triggered after the error when session is closed.
+        *
+        * EDIT(2022-05-19): The original logic was causing occasional
+        * double-free conditions once status code support was extended.
+        *
+        * Currently, we are copying the headers from ctx instead of transferring
+        * ownership, which is still a dirty workaround and, ideally, the whole
+        * logic around header (de)allocation should be reworked to make
+        * the ownership situation clear. */
+       http_free_headers(ctx->headers);
        ctx->headers = NULL;
 
        len = ctx->buf_pos - sizeof(uint16_t);
@@ -935,8 +969,6 @@ void http_free(struct http_ctx *ctx)
        while (queue_len(ctx->streams) > 0) {
                struct http_stream stream = queue_head(ctx->streams);
                http_free_headers(stream.headers);
-               if (stream.headers == ctx->headers)
-                       ctx->headers = NULL;  // to prevent double-free
                queue_pop(ctx->streams);
        }
 

diff --git a/daemon/http.h b/daemon/http.h
index 44b34820197ae4918789d57ef5c4685d7650faa6..9c34eef137e9db5983cab2ad962215807c1dd38e 100644 (file)
--- a/daemon/http.h
+++ b/daemon/http.h
@@ -82,11 +82,4 @@ int http_write(uv_write_t *req, uv_handle_t *handle, knot_pkt_t* pkt, int32_t st
               uv_write_cb on_write);
 void http_free(struct http_ctx *ctx);
 void http_free_headers(kr_http_header_array_t *headers);
-
-/** Checks if `status` has the correct `category`.
- * E.g. status 200 has category 2, status 404 has category 4, 501 has category 5 etc. */
-static inline bool http_status_has_category(enum http_status status, int category)
-{
-       return status / 100 == category;
-}
 #endif

diff --git a/daemon/worker.c b/daemon/worker.c
index 44b189809a7ffb15a4b058f9725bae90ded7b233..31feb7f779309d1d9451670404c08bf834fd7c14 100644 (file)
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -1778,9 +1778,11 @@ int worker_submit(struct session *session, struct io_comm_data *comm,
        struct http_ctx *http_ctx = NULL;
 #if ENABLE_DOH2
        http_ctx = session_http_get_server_ctx(session);
+
+       /* Badly formed query when using DoH leads to a Bad Request */
        if (http_ctx && !is_outgoing && ret) {
                http_send_status(session, HTTP_STATUS_BAD_REQUEST);
-               return kr_error(EMSGSIZE);
+               return ret;
        }
 #endif
 
@@ -1792,13 +1794,6 @@ int worker_submit(struct session *session, struct io_comm_data *comm,
            (is_query == is_outgoing)) {
                if (!is_outgoing) {
                        the_worker->stats.dropped += 1;
-               #if ENABLE_DOH2
-                       if (http_ctx) {
-                               struct http_stream stream = queue_head(http_ctx->streams);
-                               http_free_headers(stream.headers);
-                               queue_pop(http_ctx->streams);
-                       }
-               #endif
                }
                return kr_error(EILSEQ);
        }