From: Oto Šťáva Date: Fri, 13 May 2022 08:34:06 +0000 (+0200) Subject: daemon/http: copy headers to streams instead of ownership transfer X-Git-Tag: v5.5.1~11^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b9b8198142195da29a7bac4170d675f5b1987cff;p=thirdparty%2Fknot-resolver.git 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 519a0b569..f496cd70e 100644 --- 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 adb79eeb9..2e8aa2f00 100644 --- 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 44b348201..9c34eef13 100644 --- 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 44b189809..31feb7f77 100644 --- 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); }