daemon/http: move status sends outside nghttp2 callbacks

The nghttp2 documentation states that we must not send data from inside
of its callbacks. It may result in crashes.

diff --git a/daemon/http.c b/daemon/http.c
index 47dc75389922f0e7dd1a28b21730bfb4f4f8fe87..adb79eeb9ff65628b6b9bcf33d202430d11d89a2 100644 (file)
--- a/daemon/http.c
+++ b/daemon/http.c
@@ -49,19 +49,6 @@
 
 #define MAX_DECIMAL_LENGTH(VT) ((CHAR_BIT * sizeof(VT) / 3) + 3)
 
-/** HTTP status codes returned by kresd.
- * This is obviously non-exhaustive of all HTTP status codes, feel free to add
- * more if needed. */
-enum http_status {
-       HTTP_STATUS_OK                              = 200,
-       HTTP_STATUS_BAD_REQUEST                     = 400,
-       HTTP_STATUS_NOT_FOUND                       = 404,
-       HTTP_STATUS_PAYLOAD_TOO_LARGE               = 413,
-       HTTP_STATUS_UNSUPPORTED_MEDIA_TYPE          = 415,
-       HTTP_STATUS_REQUEST_HEADER_FIELDS_TOO_LARGE = 431,
-       HTTP_STATUS_NOT_IMPLEMENTED                 = 501,
-};
-
 struct http_data {
        uint8_t *buf;
        size_t len;
@@ -88,6 +75,16 @@ static ssize_t send_callback(nghttp2_session *h2, const uint8_t *data, size_t le
        return ctx->send_cb(data, length, ctx->session);
 }
 
+/*
+ * Sets the HTTP status of the specified `context`, but only if its status has
+ * not already been changed to an unsuccessful one.
+ */
+static inline void set_status(struct http_ctx *ctx, enum http_status status)
+{
+       if (http_status_has_category(ctx->status, 2))
+               ctx->status = status;
+}
+
 /*
  * Send padding length (if greater than zero).
  */
@@ -332,6 +329,7 @@ static int begin_headers_callback(nghttp2_session *h2, const nghttp2_frame *fram
        } else {
                http_cleanup_stream(ctx);  // Free any leftover data and ensure pristine state
                ctx->incomplete_stream = stream_id;
+               ctx->last_stream = stream_id;
                ctx->headers = malloc(sizeof(kr_http_header_array_t));
                array_init(*ctx->headers);
        }
@@ -371,8 +369,8 @@ static int header_callback(nghttp2_session *h2, const nghttp2_frame *frame,
                                kr_log_debug(DOH,
                                        "[%p] stream %d: header too large (%zu B), refused\n",
                                        (void *)h2, stream_id, valuelen);
-                               return http_send_response_rst_stream(ctx, stream_id, NULL,
-                                               HTTP_STATUS_REQUEST_HEADER_FIELDS_TOO_LARGE);
+                               set_status(ctx, HTTP_STATUS_REQUEST_HEADER_FIELDS_TOO_LARGE);
+                               return 0;
                        }
 
                        /* Copy the user-provided header name to keep the original case. */
@@ -392,11 +390,11 @@ static int header_callback(nghttp2_session *h2, const nghttp2_frame *frame,
        if (!strcasecmp(":path", (const char *)name)) {
                int uri_result = check_uri((const char *)value);
                if (uri_result == kr_error(ENOENT)) {
-                       return http_send_response_rst_stream(ctx, stream_id, NULL,
-                                       HTTP_STATUS_NOT_FOUND);
+                       set_status(ctx, HTTP_STATUS_NOT_FOUND);
+                       return 0;
                } else if (uri_result < 0) {
-                       return http_send_response_rst_stream(ctx, stream_id, NULL,
-                                       HTTP_STATUS_BAD_REQUEST);
+                       set_status(ctx, HTTP_STATUS_BAD_REQUEST);
+                       return 0;
                }
 
                kr_assert(ctx->uri_path == NULL);
@@ -416,15 +414,15 @@ static int header_callback(nghttp2_session *h2, const nghttp2_frame *frame,
                        ctx->current_method = HTTP_METHOD_HEAD;
                } else {
                        ctx->current_method = HTTP_METHOD_NONE;
-                       return http_send_response_rst_stream(ctx, stream_id, NULL,
-                                       HTTP_STATUS_NOT_IMPLEMENTED);
+                       set_status(ctx, HTTP_STATUS_NOT_IMPLEMENTED);
+                       return 0;
                }
        }
 
        if (!strcasecmp("content-type", (const char *)name)) {
                if (strcasecmp("application/dns-message", (const char *)value)) {
-                       return http_send_response_rst_stream(ctx, stream_id, NULL,
-                                       HTTP_STATUS_UNSUPPORTED_MEDIA_TYPE);
+                       set_status(ctx, HTTP_STATUS_UNSUPPORTED_MEDIA_TYPE);
+                       return 0;
                }
        }
 
@@ -509,16 +507,15 @@ static int submit_to_wirebuffer(struct http_ctx *ctx)
        len = ctx->buf_pos - sizeof(uint16_t);
        if (len <= 0 || len > KNOT_WIRE_MAX_PKTSIZE) {
                kr_log_debug(DOH, "[%p] invalid dnsmsg size: %zd B\n", (void *)ctx->h2, len);
-               http_send_response_rst_stream(ctx, stream_id, NULL, (len <= 0)
+               set_status(ctx, (len <= 0)
                                ? HTTP_STATUS_BAD_REQUEST
                                : HTTP_STATUS_PAYLOAD_TOO_LARGE);
-               ret = -1;
+               ret = 0;
                goto cleanup;
        }
 
        /* Submit data to wirebuffer. */
        knot_wire_write_u16(ctx->buf, len);
-       ctx->submitted_stream = stream_id;
        ctx->submitted += ctx->buf_pos;
        ctx->buf += ctx->buf_pos;
        ctx->buf_pos = 0;
@@ -549,8 +546,8 @@ static int on_frame_recv_callback(nghttp2_session *h2, const nghttp2_frame *fram
                if (ctx->current_method == HTTP_METHOD_GET || ctx->current_method == HTTP_METHOD_HEAD) {
                        if (process_uri_path(ctx, ctx->uri_path, stream_id) < 0) {
                                /* End processing - don't submit to wirebuffer. */
-                               return http_send_response_rst_stream(ctx, stream_id, NULL,
-                                               HTTP_STATUS_BAD_REQUEST);
+                               set_status(ctx, HTTP_STATUS_BAD_REQUEST);
+                               return 0;
                        }
                }
 
@@ -637,11 +634,12 @@ struct http_ctx* http_new(struct session *session, http_send_callback send_cb)
        queue_init(ctx->streams);
        ctx->stream_write_data = trie_create(NULL);
        ctx->incomplete_stream = -1;
-       ctx->submitted_stream = -1;
+       ctx->last_stream = -1;
        ctx->submitted = 0;
        ctx->streaming = true;
        ctx->current_method = HTTP_METHOD_NONE;
        ctx->uri_path = NULL;
+       ctx->status = HTTP_STATUS_OK;
 
        nghttp2_session_server_new(&ctx->h2, callbacks, ctx);
        nghttp2_submit_settings(ctx->h2, NGHTTP2_FLAG_NONE,
@@ -662,8 +660,8 @@ finish:
  * Returns 1 if stream has not ended yet, 0 if the stream has ended, or
  * a negative value on error.
  */
-int http_process_input_data(struct session *session, const uint8_t *buf,
-                           ssize_t nread, ssize_t *out_submitted)
+int http_process_input_data(struct session *session, const uint8_t *buf, ssize_t nread,
+                           ssize_t *out_submitted)
 {
        struct http_ctx *ctx = session_http_get_server_ctx(session);
        ssize_t ret = 0;
@@ -699,16 +697,23 @@ int http_process_input_data(struct session *session, const uint8_t *buf,
                return kr_error(EIO);
        }
 
+       if (!http_status_has_category(ctx->status, 2)) {
+               *out_submitted = 0;
+               http_send_status(session, ctx->status);
+               http_cleanup_stream(ctx);
+               return 0;
+       }
+
        *out_submitted = ctx->submitted;
        return ctx->streaming;
 }
 
-int http_send_bad_request(struct session *session)
+int http_send_status(struct session *session, enum http_status status)
 {
        struct http_ctx *ctx = session_http_get_server_ctx(session);
-       if (ctx->submitted_stream >= 0)
-               return http_send_response_rst_stream(ctx, ctx->submitted_stream, NULL,
-                               HTTP_STATUS_BAD_REQUEST);
+       if (ctx->last_stream >= 0)
+               return http_send_response_rst_stream(
+                               ctx, ctx->last_stream, NULL, status);
 
        return 0;
 }
@@ -845,7 +850,7 @@ static int http_send_response_rst_stream(struct http_ctx *ctx, int32_t stream_id
        if (ret)
                return ret;
 
-       ctx->submitted_stream = -1;
+       ctx->last_stream = -1;
        nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE, stream_id, NGHTTP2_NO_ERROR);
        ret = nghttp2_session_send(ctx->h2);
        return ret;

diff --git a/daemon/http.h b/daemon/http.h
index 4d37fbd60d0a95e1e3b8ad4ba0f1d4e29a63c8a0..44b34820197ae4918789d57ef5c4685d7650faa6 100644 (file)
--- a/daemon/http.h
+++ b/daemon/http.h
@@ -40,6 +40,19 @@ typedef enum {
                               * Required to be implemented by RFC 7231. */
 } http_method_t;
 
+/** HTTP status codes returned by kresd.
+ * This is obviously non-exhaustive of all HTTP status codes, feel free to add
+ * more if needed. */
+enum http_status {
+       HTTP_STATUS_OK                              = 200,
+       HTTP_STATUS_BAD_REQUEST                     = 400,
+       HTTP_STATUS_NOT_FOUND                       = 404,
+       HTTP_STATUS_PAYLOAD_TOO_LARGE               = 413,
+       HTTP_STATUS_UNSUPPORTED_MEDIA_TYPE          = 415,
+       HTTP_STATUS_REQUEST_HEADER_FIELDS_TOO_LARGE = 431,
+       HTTP_STATUS_NOT_IMPLEMENTED                 = 501,
+};
+
 struct http_ctx {
        struct nghttp2_session *h2;
        http_send_callback send_cb;
@@ -47,7 +60,8 @@ struct http_ctx {
        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;
-       int32_t submitted_stream;   /* Stream whose data has been submitted to the wire buffer. */
+       int32_t last_stream;   /* The last used stream - mostly the same as incomplete_stream, but can be used after
+                                 completion for sending HTTP status codes. */
        ssize_t submitted;
        http_method_t current_method;
        char *uri_path;
@@ -55,16 +69,24 @@ struct http_ctx {
        uint8_t *buf;  /* Part of the wire_buf that belongs to current HTTP/2 stream. */
        ssize_t buf_pos;
        ssize_t buf_size;
+       enum http_status status;
        bool streaming;             /* True: not all data in the stream has been received yet. */
 };
 
 #if ENABLE_DOH2
 struct http_ctx* http_new(struct session *session, http_send_callback send_cb);
-int http_process_input_data(struct session *session, const uint8_t *buf,
-                           ssize_t nread, ssize_t *out_submitted);
-int http_send_bad_request(struct session *session);
+int http_process_input_data(struct session *session, const uint8_t *buf, ssize_t nread,
+                           ssize_t *out_submitted);
+int http_send_status(struct session *session, enum http_status status);
 int http_write(uv_write_t *req, uv_handle_t *handle, knot_pkt_t* pkt, int32_t stream_id,
               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/io.c b/daemon/io.c
index 30b3f453ef2d22fc545e7d0c3155f2439e7191da..e1e5cac357cc2990def417476292640a83fbd49d 100644 (file)
--- a/daemon/io.c
+++ b/daemon/io.c
@@ -438,7 +438,8 @@ static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf)
 #if ENABLE_DOH2
        int streaming = 1;
        if (session_flags(s)->has_http) {
-               streaming = http_process_input_data(s, data, data_len, &consumed);
+               streaming = http_process_input_data(s, data, data_len,
+                               &consumed);
                if (streaming < 0) {
                        if (kr_log_is_debug(IO, NULL)) {
                                char *peer_str = kr_straddr(src_addr);
@@ -477,7 +478,7 @@ static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf)
        mp_flush(the_worker->pkt_pool.ctx);
 #if ENABLE_DOH2
        if (session_flags(s)->has_http && streaming == 0 && ret == 0) {
-               ret = http_send_bad_request(s);
+               ret = http_send_status(s, HTTP_STATUS_BAD_REQUEST);
                if (ret) {
                        /* An error has occurred, close the session. */
                        worker_end_tcp(s);

diff --git a/daemon/worker.c b/daemon/worker.c
index 156335d314047fe5fb33ebb6292fdcc11cf50620..44b189809a7ffb15a4b058f9725bae90ded7b233 100644 (file)
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -1779,7 +1779,7 @@ int worker_submit(struct session *session, struct io_comm_data *comm,
 #if ENABLE_DOH2
        http_ctx = session_http_get_server_ctx(session);
        if (http_ctx && !is_outgoing && ret) {
-               http_send_bad_request(session);
+               http_send_status(session, HTTP_STATUS_BAD_REQUEST);
                return kr_error(EMSGSIZE);
        }
 #endif

diff --git a/tests/config/doh2.test.lua b/tests/config/doh2.test.lua
index 352ea42834285d6c90410932aa6a005f1f6d782d..881515c7b2b52762ecc93f965708c53c9de12d8b 100644 (file)
--- a/tests/config/doh2.test.lua
+++ b/tests/config/doh2.test.lua
@@ -280,7 +280,7 @@ else
        end
 
        -- test an invalid DNS query using GET
-               local function test_get_long_input()
+       local function test_get_long_input()
                local req = assert(req_templ:clone())
                req.headers:upsert(':method', 'GET')
                req.headers:upsert(':path', '/doh?dns=' .. basexx.to_url64(string.rep('\0', 1030)))