Keep the endpoints set reference within an HTTP/2 socket

This commit ensures that an HTTP endpoints set reference is stored in
a socket object associated with an HTTP/2 stream instead of
referencing the global set stored inside a listener.

This helps to prevent an issue like follows:

1. BIND is configured to serve DoH clients;
2. A client is connected and one or more HTTP/2 stream is
created. Internal pointers are now pointing to the data on the
associated HTTP endpoints set;
3. BIND is reconfigured - the new endpoints set object is created and
promoted to all listeners;
4. The old pointers to the HTTP endpoints set data are now invalid.

Instead referencing a global object that is updated on
re-configurations we now store a local reference which prevents the
endpoints set objects to go out of scope prematurely.

diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c
index 42e2a9fda1190d3e7cf5d2706c6a40c1220d7e1b..4dbf6080df7bc79ded7cb2876e7b45996701a4cb 100644 (file)
--- a/lib/isc/netmgr/http.c
+++ b/lib/isc/netmgr/http.c
@@ -182,6 +182,9 @@ typedef struct isc_http_send_req {
 #define HTTP_ENDPOINTS_MAGIC   ISC_MAGIC('H', 'T', 'E', 'P')
 #define VALID_HTTP_ENDPOINTS(t) ISC_MAGIC_VALID(t, HTTP_ENDPOINTS_MAGIC)
 
+#define HTTP_HANDLER_MAGIC    ISC_MAGIC('H', 'T', 'H', 'L')
+#define VALID_HTTP_HANDLER(t) ISC_MAGIC_VALID(t, HTTP_HANDLER_MAGIC)
+
 static bool
 http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle,
                   isc_nm_cb_t cb, void *cbarg);
@@ -1714,6 +1717,9 @@ server_on_begin_headers_callback(nghttp2_session *ngsession,
        };
        isc_buffer_initnull(&socket->h2->rbuf);
        isc_buffer_initnull(&socket->h2->wbuf);
+       isc_nm_http_endpoints_attach(
+               http_get_listener_endpoints(session->serversocket, socket->tid),
+               &socket->h2->peer_endpoints);
        session->nsstreams++;
        isc__nm_httpsession_attach(session, &socket->h2->session);
        ISC_LIST_APPEND(session->sstreams, socket->h2, link);
@@ -1723,19 +1729,6 @@ server_on_begin_headers_callback(nghttp2_session *ngsession,
        return (0);
 }
 
-static isc_nm_httphandler_t *
-find_server_request_handler(const char *request_path,
-                           isc_nmsocket_t *serversocket, const int tid) {
-       isc_nm_httphandler_t *handler = NULL;
-
-       REQUIRE(VALID_NMSOCK(serversocket));
-
-       handler = http_endpoints_find(
-               request_path, http_get_listener_endpoints(serversocket, tid));
-
-       return (handler);
-}
-
 static isc_http_error_responses_t
 server_handle_path_header(isc_nmsocket_t *socket, const uint8_t *value,
                          const size_t valuelen) {
@@ -1760,9 +1753,8 @@ server_handle_path_header(isc_nmsocket_t *socket, const uint8_t *value,
                return (ISC_HTTP_ERROR_BAD_REQUEST);
        }
 
-       handler = find_server_request_handler(socket->h2->request_path,
-                                             socket->h2->session->serversocket,
-                                             socket->tid);
+       handler = http_endpoints_find(socket->h2->request_path,
+                                     socket->h2->peer_endpoints);
        if (handler != NULL) {
                socket->h2->cb = handler->cb;
                socket->h2->cbarg = handler->cbarg;
@@ -2085,9 +2077,20 @@ server_call_cb(isc_nmsocket_t *socket, const isc_result_t result,
        isc_nmhandle_t *handle = NULL;
 
        REQUIRE(VALID_NMSOCK(socket));
-       REQUIRE(socket->h2->cb != NULL);
+
+       /*
+        * In some cases the callback could not have been set (e.g. when
+        * the stream was closed prematurely (before processing its HTTP
+        * path).
+        */
+       if (socket->h2->cb == NULL) {
+               return;
+       }
 
        handle = isc__nmhandle_get(socket, NULL, NULL);
+       if (result != ISC_R_SUCCESS) {
+               data = NULL;
+       }
        socket->h2->cb(handle, result, data, socket->h2->cbarg);
        isc_nmhandle_detach(&handle);
 }
@@ -2505,7 +2508,6 @@ isc_nm_listenhttp(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface,
 
        REQUIRE(VALID_NM(mgr));
        REQUIRE(!ISC_LIST_EMPTY(eps->handlers));
-       REQUIRE(!ISC_LIST_EMPTY(eps->handler_cbargs));
        REQUIRE(atomic_load(&eps->in_use) == false);
        REQUIRE(isc_tid() == 0);
 
@@ -2576,7 +2578,6 @@ isc_nm_http_endpoints_new(isc_mem_t *mctx) {
        *eps = (isc_nm_http_endpoints_t){ .mctx = NULL };
 
        isc_mem_attach(mctx, &eps->mctx);
-       ISC_LIST_INIT(eps->handler_cbargs);
        ISC_LIST_INIT(eps->handlers);
        isc_refcount_init(&eps->references, 1);
        atomic_init(&eps->in_use, false);
@@ -2590,7 +2591,6 @@ isc_nm_http_endpoints_detach(isc_nm_http_endpoints_t **restrict epsp) {
        isc_nm_http_endpoints_t *restrict eps;
        isc_mem_t *mctx;
        isc_nm_httphandler_t *handler = NULL;
-       isc_nm_httpcbarg_t *httpcbarg = NULL;
 
        REQUIRE(epsp != NULL);
        eps = *epsp;
@@ -2611,20 +2611,11 @@ isc_nm_http_endpoints_detach(isc_nm_http_endpoints_t **restrict epsp) {
                next = ISC_LIST_NEXT(handler, link);
                ISC_LIST_DEQUEUE(eps->handlers, handler, link);
                isc_mem_free(mctx, handler->path);
+               handler->magic = 0;
                isc_mem_put(mctx, handler, sizeof(*handler));
                handler = next;
        }
 
-       httpcbarg = ISC_LIST_HEAD(eps->handler_cbargs);
-       while (httpcbarg != NULL) {
-               isc_nm_httpcbarg_t *next = NULL;
-
-               next = ISC_LIST_NEXT(httpcbarg, link);
-               ISC_LIST_DEQUEUE(eps->handler_cbargs, httpcbarg, link);
-               isc_mem_put(mctx, httpcbarg, sizeof(isc_nm_httpcbarg_t));
-               httpcbarg = next;
-       }
-
        eps->magic = 0;
 
        isc_mem_putanddetach(&mctx, eps, sizeof(*eps));
@@ -2657,6 +2648,8 @@ http_endpoints_find(const char *request_path,
             handler = ISC_LIST_NEXT(handler, link))
        {
                if (!strcmp(request_path, handler->path)) {
+                       INSIST(VALID_HTTP_HANDLER(handler));
+                       INSIST(handler->cb != NULL);
                        break;
                }
        }
@@ -2664,61 +2657,33 @@ http_endpoints_find(const char *request_path,
        return (handler);
 }
 
-/*
- * In DoH we just need to intercept the request - the response can be sent
- * to the client code via the nmhandle directly as it's always just the
- * http content.
- */
-static void
-http_callback(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *data,
-             void *arg) {
-       isc_nm_httpcbarg_t *httpcbarg = arg;
-
-       REQUIRE(VALID_NMHANDLE(handle));
-
-       if (result != ISC_R_SUCCESS) {
-               /* Shut down the client, then ourselves */
-               httpcbarg->cb(handle, result, NULL, httpcbarg->cbarg);
-               /* XXXWPK FREE */
-               return;
-       }
-       httpcbarg->cb(handle, result, data, httpcbarg->cbarg);
-}
-
 isc_result_t
 isc_nm_http_endpoints_add(isc_nm_http_endpoints_t *restrict eps,
                          const char *uri, const isc_nm_recv_cb_t cb,
                          void *cbarg) {
        isc_mem_t *mctx;
        isc_nm_httphandler_t *restrict handler = NULL;
-       isc_nm_httpcbarg_t *restrict httpcbarg = NULL;
-       bool newhandler = false;
 
        REQUIRE(VALID_HTTP_ENDPOINTS(eps));
        REQUIRE(isc_nm_http_path_isvalid(uri));
+       REQUIRE(cb != NULL);
        REQUIRE(atomic_load(&eps->in_use) == false);
 
        mctx = eps->mctx;
 
-       httpcbarg = isc_mem_get(mctx, sizeof(isc_nm_httpcbarg_t));
-       *httpcbarg = (isc_nm_httpcbarg_t){ .cb = cb, .cbarg = cbarg };
-       ISC_LINK_INIT(httpcbarg, link);
-
        if (http_endpoints_find(uri, eps) == NULL) {
                handler = isc_mem_get(mctx, sizeof(*handler));
-               *handler = (isc_nm_httphandler_t){ .cb = http_callback,
-                                                  .cbarg = httpcbarg,
-                                                  .path = isc_mem_strdup(
-                                                          mctx, uri) };
-               ISC_LINK_INIT(handler, link);
-
-               newhandler = true;
-       }
+               *handler = (isc_nm_httphandler_t){
+                       .cb = cb,
+                       .cbarg = cbarg,
+                       .path = isc_mem_strdup(mctx, uri),
+                       .link = ISC_LINK_INITIALIZER,
+                       .magic = HTTP_HANDLER_MAGIC
+               };
 
-       if (newhandler) {
                ISC_LIST_APPEND(eps->handlers, handler, link);
        }
-       ISC_LIST_APPEND(eps->handler_cbargs, httpcbarg, link);
+
        return (ISC_R_SUCCESS);
 }
 
@@ -2802,8 +2767,6 @@ failed_httpstream_read_cb(isc_nmsocket_t *sock, isc_result_t result,
                return;
        }
 
-       INSIST(sock->h2->cbarg != NULL);
-
        (void)nghttp2_submit_rst_stream(
                session->ngsession, NGHTTP2_FLAG_END_STREAM,
                sock->h2->stream_id, NGHTTP2_REFUSED_STREAM);
@@ -3252,6 +3215,12 @@ isc__nm_http_cleanup_data(isc_nmsocket_t *sock) {
                        http_cleanup_listener_endpoints(sock);
                }
 
+               if (sock->type == isc_nm_httpsocket &&
+                   sock->h2->peer_endpoints != NULL)
+               {
+                       isc_nm_http_endpoints_detach(&sock->h2->peer_endpoints);
+               }
+
                if (sock->h2->request_path != NULL) {
                        isc_mem_free(sock->worker->mctx,
                                     sock->h2->request_path);

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 13485dfb58d0cb3dae5edc9c50f6fb4f3579ed28..56e5bef4a9589549cf47b66a8ab1d042b2499c68 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -410,13 +410,8 @@ typedef enum isc_http_scheme_type {
        ISC_HTTP_SCHEME_UNSUPPORTED
 } isc_http_scheme_type_t;
 
-typedef struct isc_nm_httpcbarg {
-       isc_nm_recv_cb_t cb;
-       void *cbarg;
-       LINK(struct isc_nm_httpcbarg) link;
-} isc_nm_httpcbarg_t;
-
 typedef struct isc_nm_httphandler {
+       int magic;
        char *path;
        isc_nm_recv_cb_t cb;
        void *cbarg;
@@ -428,7 +423,6 @@ struct isc_nm_http_endpoints {
        isc_mem_t *mctx;
 
        ISC_LIST(isc_nm_httphandler_t) handlers;
-       ISC_LIST(isc_nm_httpcbarg_t) handler_cbargs;
 
        isc_refcount_t references;
        atomic_bool in_use;
@@ -440,7 +434,6 @@ typedef struct isc_nmsocket_h2 {
        char *query_data;
        size_t query_data_len;
        bool query_too_large;
-       isc_nm_httphandler_t *handler;
 
        isc_buffer_t rbuf;
        isc_buffer_t wbuf;
@@ -471,6 +464,8 @@ typedef struct isc_nmsocket_h2 {
        isc_nm_http_endpoints_t **listener_endpoints;
        size_t n_listener_endpoints;
 
+       isc_nm_http_endpoints_t *peer_endpoints;
+
        bool response_submitted;
        struct {
                char *uri;