From c819caa3a16ec429c284252f9a32ee4220eccc55 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 19 Jul 2021 15:20:30 +0300 Subject: [PATCH 01/15] Replace the HTTP/2 session's ad-hoc buffer with isc_buffer_t This commit replaces a static ad-hoc HTTP/2 session's temporary buffer with a realloc-able isc_buffer_t object, which is being allocated on as needed basis, lowering the memory consumption somewhat. The buffer is needed in very rare cases, so allocating it prematurely is not wise. Also, it fixes a bug in http_readcb() where the ad-hoc buffer appeared to be improperly used, leading to a situation when the processed data from the receiving regions can be processed twice, while unprocessed data will never be processed. --- lib/isc/netmgr/http.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 4d19593aef..d32b908191 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -139,8 +139,7 @@ struct isc_nm_http_session { isc_nmhandle_t *client_httphandle; isc_nmsocket_t *serversocket; - uint8_t buf[MAX_DNS_MESSAGE_SIZE]; - size_t bufsize; + isc_buffer_t *buf; isc_tlsctx_t *tlsctx; uint32_t max_concurrent_streams; @@ -322,6 +321,10 @@ isc__nm_httpsession_detach(isc_nm_http_session_t **sessionp) { session->ngsession = NULL; } + if (session->buf != NULL) { + isc_buffer_free(&session->buf); + } + /* We need an acquire memory barrier here */ (void)isc_refcount_current(&session->references); @@ -956,10 +959,14 @@ http_readcb(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, } if ((size_t)readlen < region->length) { - INSIST(session->bufsize == 0); - INSIST(region->length - readlen < MAX_DNS_MESSAGE_SIZE); - memmove(session->buf, region->base, region->length - readlen); - session->bufsize = region->length - readlen; + size_t unread_size = region->length - readlen; + if (session->buf == NULL) { + isc_buffer_allocate(session->mctx, &session->buf, + unread_size); + isc_buffer_setautorealloc(session->buf, true); + } + isc_buffer_putmem(session->buf, region->base + readlen, + unread_size); isc_nm_pauseread(session->handle); } @@ -1244,18 +1251,18 @@ http_do_bio(isc_nm_http_session_t *session, isc_nmhandle_t *send_httphandle, /* We have not yet started reading from this handle */ isc_nm_read(session->handle, http_readcb, session); session->reading = true; - } else if (session->bufsize > 0) { + } else if (session->buf != NULL) { + size_t remaining = + isc_buffer_remaininglength(session->buf); /* Leftover data in the buffer, use it */ size_t readlen = nghttp2_session_mem_recv( - session->ngsession, session->buf, - session->bufsize); + session->ngsession, + isc_buffer_current(session->buf), remaining); - if (readlen == session->bufsize) { - session->bufsize = 0; + if (readlen == remaining) { + isc_buffer_free(&session->buf); } else { - memmove(session->buf, session->buf + readlen, - session->bufsize - readlen); - session->bufsize -= readlen; + isc_buffer_forward(session->buf, readlen); } http_do_bio(session, send_httphandle, send_cb, From 2733cca3ace985822b62b5213983861e46f6f018 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 19 Jul 2021 20:55:12 +0300 Subject: [PATCH 02/15] Replace ad-hoc DNS message buffer in client code with isc_buffer_t The commit replaces an ad-hoc incoming DNS-message buffer in the client-side DoH code with isc_buffer_t. The commit also fixes a timing issue in the unit tests revealed by the change. --- lib/isc/netmgr/http.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index d32b908191..f3dabbd610 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -74,6 +74,8 @@ ((code) >= MIN_SUCCESSFUL_HTTP_STATUS && \ (code) <= MAX_SUCCESSFUL_HTTP_STATUS) +#define INITIAL_DNS_MESSAGE_BUFFER_SIZE (512) + typedef struct isc_nm_http_response_status { size_t code; size_t content_length; @@ -96,8 +98,7 @@ typedef struct http_cstream { size_t authoritylen; char *path; - uint8_t rbuf[MAX_DNS_MESSAGE_SIZE]; - size_t rbufsize; + isc_buffer_t *rbuf; size_t pathlen; int32_t stream_id; @@ -426,6 +427,10 @@ new_http_cstream(isc_nmsocket_t *sock, http_cstream_t **streamp) { stream->up.field_data[ISC_UF_QUERY].len); } + isc_buffer_allocate(mctx, &stream->rbuf, + INITIAL_DNS_MESSAGE_BUFFER_SIZE); + isc_buffer_setautorealloc(stream->rbuf, true); + ISC_LIST_PREPEND(sock->h2.session->cstreams, stream, link); *streamp = stream; @@ -455,6 +460,8 @@ put_http_cstream(isc_mem_t *mctx, http_cstream_t *stream) { link); } isc__nmsocket_detach(&stream->httpsock); + + isc_buffer_free(&stream->rbuf); isc_mem_put(mctx, stream, sizeof(http_cstream_t)); } @@ -506,12 +513,13 @@ on_client_data_chunk_recv_callback(int32_t stream_id, const uint8_t *data, http_cstream_t *cstream = find_http_cstream(stream_id, session); if (cstream != NULL) { - size_t new_rbufsize = cstream->rbufsize + len; + size_t new_rbufsize = len; + INSIST(cstream->rbuf != NULL); + new_rbufsize += isc_buffer_usedlength(cstream->rbuf); if (new_rbufsize <= MAX_DNS_MESSAGE_SIZE && new_rbufsize <= cstream->response_status.content_length) { - memmove(cstream->rbuf + cstream->rbufsize, data, len); - cstream->rbufsize = new_rbufsize; + isc_buffer_putmem(cstream->rbuf, data, len); } else { return (NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE); } @@ -572,12 +580,13 @@ static void call_unlink_cstream_readcb(http_cstream_t *cstream, isc_nm_http_session_t *session, isc_result_t result) { + isc_region_t read_data; REQUIRE(VALID_HTTP2_SESSION(session)); REQUIRE(cstream != NULL); ISC_LIST_UNLINK(session->cstreams, cstream, link); INSIST(VALID_NMHANDLE(session->client_httphandle)); - cstream->read_cb(session->client_httphandle, result, - &(isc_region_t){ cstream->rbuf, cstream->rbufsize }, + isc_buffer_usedregion(cstream->rbuf, &read_data); + cstream->read_cb(session->client_httphandle, result, &read_data, cstream->read_cbarg); put_http_cstream(session->mctx, cstream); } @@ -2760,10 +2769,10 @@ client_call_failed_read_cb(isc_result_t result, * in such a case. */ if (cstream->read_cb != NULL) { + isc_region_t read_data; + isc_buffer_usedregion(cstream->rbuf, &read_data); cstream->read_cb(session->client_httphandle, result, - &(isc_region_t){ cstream->rbuf, - cstream->rbufsize }, - cstream->read_cbarg); + &read_data, cstream->read_cbarg); } if (result != ISC_R_TIMEDOUT || cstream->read_cb == NULL || From 0ca790d9bfb21071d4f973df2e8883cc69d7598d Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 21 Jul 2021 20:03:44 +0300 Subject: [PATCH 03/15] DoH: isc__buffer_usedregion->isc_buffer_usedregion in client_send() This commit replaces wrong usage of isc__buffer_usedregion() instead of implied isc_buffer_usedregion(). --- lib/isc/netmgr/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index f3dabbd610..2234f0d56d 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1560,7 +1560,7 @@ client_send(isc_nmhandle_t *handle, const isc_region_t *region) { goto error; } - isc__buffer_usedregion(buf, &base64_region); + isc_buffer_usedregion(buf, &base64_region); INSIST(base64_region.length == base64_len); base64url_data = isc__nm_base64_to_base64url( From 6fe4ab39b909d6fa4ed1a3b3384b4cb07e1112b9 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 21 Jul 2021 20:10:46 +0300 Subject: [PATCH 04/15] Use isc_buffer_t to keep track of incoming POST data This commit replaces the ad-hoc 64K buffer for incoming POST data with isc_buffer_t backed by dynamically allocated buffer sized accordingly to the value in the "Content-Length" header. --- lib/isc/netmgr/http.c | 40 +++++++++++++++++++++++++++---------- lib/isc/netmgr/netmgr-int.h | 1 + 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 2234f0d56d..4721bd71bd 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -536,11 +536,18 @@ on_server_data_chunk_recv_callback(int32_t stream_id, const uint8_t *data, isc_nmsocket_h2_t *h2 = ISC_LIST_HEAD(session->sstreams); while (h2 != NULL) { if (stream_id == h2->stream_id) { - size_t new_bufsize = h2->bufsize + len; + if (isc_buffer_base(&h2->rbuf) == NULL) { + isc_buffer_init( + &h2->rbuf, + isc_mem_allocate(session->mctx, + h2->content_length), + MAX_DNS_MESSAGE_SIZE); + } + size_t new_bufsize = isc_buffer_usedlength(&h2->rbuf) + + len; if (new_bufsize <= MAX_DNS_MESSAGE_SIZE && new_bufsize <= h2->content_length) { - memmove(h2->buf + h2->bufsize, data, len); - h2->bufsize = new_bufsize; + isc_buffer_putmem(&h2->rbuf, data, len); break; } @@ -1660,6 +1667,7 @@ server_on_begin_headers_callback(nghttp2_session *ngsession, .stream_id = frame->hd.stream_id, .headers_error_code = ISC_HTTP_ERROR_SUCCESS }; + isc_buffer_initnull(&socket->h2.rbuf); session->nsstreams++; isc__nm_httpsession_attach(session, &socket->h2.session); socket->tid = session->handle->sock->tid; @@ -1990,8 +1998,11 @@ static struct http_error_responses { static isc_result_t server_send_error_response(const isc_http_error_responses_t error, nghttp2_session *ngsession, isc_nmsocket_t *socket) { - socket->h2.bufsize = 0; - socket->h2.bufpos = 0; + void *base = isc_buffer_base(&socket->h2.rbuf); + if (base != NULL) { + isc_mem_free(socket->h2.session->mctx, base); + isc_buffer_initnull(&socket->h2.rbuf); + } for (size_t i = 0; i < sizeof(error_responses) / sizeof(error_responses[0]); i++) @@ -2053,11 +2064,13 @@ server_on_request_recv(nghttp2_session *ngsession, if (socket->h2.request_path == NULL || socket->h2.cb == NULL) { code = ISC_HTTP_ERROR_NOT_FOUND; } else if (socket->h2.request_type == ISC_HTTP_REQ_POST && - socket->h2.bufsize > socket->h2.content_length) + isc_buffer_usedlength(&socket->h2.rbuf) > + socket->h2.content_length) { code = ISC_HTTP_ERROR_PAYLOAD_TOO_LARGE; } else if (socket->h2.request_type == ISC_HTTP_REQ_POST && - socket->h2.bufsize != socket->h2.content_length) + isc_buffer_usedlength(&socket->h2.rbuf) != + socket->h2.content_length) { code = ISC_HTTP_ERROR_BAD_REQUEST; } @@ -2079,7 +2092,7 @@ server_on_request_recv(nghttp2_session *ngsession, isc__buffer_usedregion(&decoded_buf, &data); } else if (socket->h2.request_type == ISC_HTTP_REQ_POST) { INSIST(socket->h2.content_length > 0); - data = (isc_region_t){ socket->h2.buf, socket->h2.bufsize }; + isc_buffer_usedregion(&socket->h2.rbuf, &data); } else { INSIST(0); ISC_UNREACHABLE(); @@ -2734,6 +2747,7 @@ isc__nm_async_httpclose(isc__networker_t *worker, isc__netievent_t *ev0) { static void failed_httpstream_read_cb(isc_nmsocket_t *sock, isc_result_t result, isc_nm_http_session_t *session) { + isc_region_t data; REQUIRE(VALID_NMSOCK(sock)); INSIST(sock->type == isc_nm_httpsocket); @@ -2746,8 +2760,8 @@ failed_httpstream_read_cb(isc_nmsocket_t *sock, isc_result_t result, (void)nghttp2_submit_rst_stream( session->ngsession, NGHTTP2_FLAG_END_STREAM, sock->h2.stream_id, NGHTTP2_REFUSED_STREAM); - server_call_cb(sock, session, result, - &(isc_region_t){ sock->h2.buf, sock->h2.bufsize }); + isc_buffer_usedregion(&sock->h2.rbuf, &data); + server_call_cb(sock, session, result, &data); } static void @@ -3015,6 +3029,12 @@ isc__nm_http_cleanup_data(isc_nmsocket_t *sock) { isc_mem_free(sock->mgr->mctx, sock->h2.buf); sock->h2.buf = NULL; } + + if (isc_buffer_base(&sock->h2.rbuf) != NULL) { + void *base = isc_buffer_base(&sock->h2.rbuf); + isc_mem_free(sock->mgr->mctx, base); + isc_buffer_initnull(&sock->h2.rbuf); + } } if ((sock->type == isc_nm_httplistener || diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index f46e7fce5a..857c34aaa1 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -803,6 +803,7 @@ typedef struct isc_nmsocket_h2 { uint8_t *buf; size_t bufsize; size_t bufpos; + isc_buffer_t rbuf; int32_t stream_id; isc_nm_http_session_t *session; From e0704f2e5df9dd09b90f264520a13fd63e0efc5f Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 21 Jul 2021 23:23:58 +0300 Subject: [PATCH 05/15] Use isc_buffer_t to keep track of outgoing response This commit gets rid of custom code taking care of response buffering by replacing the custom code with isc_buffer_t. Also, it gets rid of an unnecessary memory copying when sending a response. --- lib/isc/netmgr/http.c | 38 ++++++++++++++++++------------------- lib/isc/netmgr/netmgr-int.h | 4 +--- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 4721bd71bd..3542f9f68e 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1661,13 +1661,12 @@ server_on_begin_headers_callback(nghttp2_session *ngsession, isc_nm_httpsocket, (isc_sockaddr_t *)&session->handle->sock->iface); socket->peer = session->handle->sock->peer; - socket->h2 = (isc_nmsocket_h2_t){ - .buf = isc_mem_allocate(session->mctx, MAX_DNS_MESSAGE_SIZE), - .psock = socket, - .stream_id = frame->hd.stream_id, - .headers_error_code = ISC_HTTP_ERROR_SUCCESS - }; + socket->h2 = (isc_nmsocket_h2_t){ .psock = socket, + .stream_id = frame->hd.stream_id, + .headers_error_code = + ISC_HTTP_ERROR_SUCCESS }; isc_buffer_initnull(&socket->h2.rbuf); + isc_buffer_initnull(&socket->h2.wbuf); session->nsstreams++; isc__nm_httpsession_attach(session, &socket->h2.session); socket->tid = session->handle->sock->tid; @@ -1928,14 +1927,18 @@ server_read_callback(nghttp2_session *ngsession, int32_t stream_id, UNUSED(ngsession); UNUSED(session); - buflen = socket->h2.bufsize - socket->h2.bufpos; + buflen = isc_buffer_remaininglength(&socket->h2.wbuf); if (buflen > length) { buflen = length; } - memmove(buf, socket->h2.buf + socket->h2.bufpos, buflen); - socket->h2.bufpos += buflen; - if (socket->h2.bufpos == socket->h2.bufsize) { + if (buflen > 0) { + (void)memmove(buf, isc_buffer_current(&socket->h2.wbuf), + buflen); + isc_buffer_forward(&socket->h2.wbuf, buflen); + } + + if (isc_buffer_remaininglength(&socket->h2.wbuf) == 0) { *data_flags |= NGHTTP2_DATA_FLAG_EOF; } @@ -2055,6 +2058,7 @@ server_on_request_recv(nghttp2_session *ngsession, isc_result_t result; isc_http_error_responses_t code = ISC_HTTP_ERROR_SUCCESS; isc_region_t data; + uint8_t tmp_buf[MAX_DNS_MESSAGE_SIZE]; code = socket->h2.headers_error_code; if (code != ISC_HTTP_ERROR_SUCCESS) { @@ -2081,15 +2085,14 @@ server_on_request_recv(nghttp2_session *ngsession, if (socket->h2.request_type == ISC_HTTP_REQ_GET) { isc_buffer_t decoded_buf; - isc__buffer_init(&decoded_buf, socket->h2.buf, - MAX_DNS_MESSAGE_SIZE); + isc_buffer_init(&decoded_buf, tmp_buf, sizeof(tmp_buf)); if (isc_base64_decodestring(socket->h2.query_data, &decoded_buf) != ISC_R_SUCCESS) { code = ISC_HTTP_ERROR_BAD_REQUEST; goto error; } - isc__buffer_usedregion(&decoded_buf, &data); + isc_buffer_usedregion(&decoded_buf, &data); } else if (socket->h2.request_type == ISC_HTTP_REQ_POST) { INSIST(socket->h2.content_length > 0); isc_buffer_usedregion(&socket->h2.rbuf, &data); @@ -2184,8 +2187,8 @@ server_httpsend(isc_nmhandle_t *handle, isc_nmsocket_t *sock, INSIST(VALID_NMHANDLE(handle->httpsession->handle)); INSIST(VALID_NMSOCK(handle->httpsession->handle->sock)); - memmove(sock->h2.buf, req->uvbuf.base, req->uvbuf.len); - sock->h2.bufsize = req->uvbuf.len; + isc_buffer_init(&sock->h2.wbuf, req->uvbuf.base, req->uvbuf.len); + isc_buffer_add(&sock->h2.wbuf, req->uvbuf.len); len = snprintf(sock->h2.clenbuf, sizeof(sock->h2.clenbuf), "%lu", (unsigned long)req->uvbuf.len); @@ -3025,11 +3028,6 @@ isc__nm_http_cleanup_data(isc_nmsocket_t *sock) { INSIST(sock->h2.connect.cstream == NULL); - if (sock->h2.buf != NULL) { - isc_mem_free(sock->mgr->mctx, sock->h2.buf); - sock->h2.buf = NULL; - } - if (isc_buffer_base(&sock->h2.rbuf) != NULL) { void *base = isc_buffer_base(&sock->h2.rbuf); isc_mem_free(sock->mgr->mctx, base); diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 857c34aaa1..c75ddddfb8 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -800,10 +800,8 @@ typedef struct isc_nmsocket_h2 { bool query_too_large; isc_nm_httphandler_t *handler; - uint8_t *buf; - size_t bufsize; - size_t bufpos; isc_buffer_t rbuf; + isc_buffer_t wbuf; int32_t stream_id; isc_nm_http_session_t *session; From 849d38b57b2b9d73356e11d7984e6929dc6e4805 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 22 Jul 2021 00:04:02 +0300 Subject: [PATCH 06/15] Fix a crash by attach to the transport socket as early as possible This commit fixes a crash in DoH caused by transport handle to be detached too early when sending outgoing data. We need to attach to the session->handle earlier because as an indirect result of the nghttp2_session_mem_send() the session might get closed and the handle detached. However, there is still might be some outgoing data to handle. Besides, even when the underlying socket was closed via the handle, we still should try to attempt to send outgoing data via isc_nm_send() to let it call write callback, passed to the http_send_outgoing(). --- lib/isc/netmgr/http.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 3542f9f68e..4e77a2ba82 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1054,6 +1054,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, size_t total = 0; uint8_t tmp_data[8192] = { 0 }; uint8_t *prepared_data = &tmp_data[0]; + isc_nmhandle_t *transphandle = NULL; #ifdef ENABLE_HTTP_WRITE_BUFFERING size_t max_total_write_size = 0; #endif /* ENABLE_HTTP_WRITE_BUFFERING */ @@ -1065,6 +1066,14 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, return (false); } + /* We need to attach to the session->handle earlier because as an + * indirect result of the nghttp2_session_mem_send() the session + * might get closed and the handle detached. However, there is + * still some outgoing data to handle and we need to call it + * anyway if only to get the write callback passed here to get + * called properly. */ + isc_nmhandle_attach(session->handle, &transphandle); + while (nghttp2_session_want_write(session->ngsession)) { const uint8_t *data = NULL; const size_t pending = @@ -1159,7 +1168,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, if (prepared_data != &tmp_data[0]) { isc_mem_put(session->mctx, prepared_data, total); } - return (false); + goto failure; } else if (session->sending == 0 && total == 0 && session->pending_write_data != NULL) { @@ -1198,7 +1207,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, if (total == 0) { INSIST(prepared_data == &tmp_data[0]); /* No data returned */ - return (false); + goto failure; } send = isc_mem_get(session->mctx, sizeof(*send)); @@ -1222,7 +1231,8 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, .data.length = total, }; } - isc_nmhandle_attach(session->handle, &send->transphandle); + + send->transphandle = transphandle; isc__nm_httpsession_attach(session, &send->session); if (cb != NULL) { @@ -1235,8 +1245,11 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, move_pending_send_callbacks(session, send); session->sending++; - isc_nm_send(session->handle, &send->data, http_writecb, send); + isc_nm_send(transphandle, &send->data, http_writecb, send); return (true); +failure: + isc_nmhandle_detach(&transphandle); + return (false); } static void From a05728beb0155049abb4ec913f40e651c1ba7973 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 28 Jul 2021 16:44:46 +0300 Subject: [PATCH 07/15] Do not call http_do_bio() in isc__nm_http_request() The function should not be called here because it is, in general, supposed to be called at the end of the transport level callbacks to perform I/O, and thus, calling it here is clearly a mistake because it breaks other code expectations. As a result of the call to http_do_bio() from within isc__nm_http_request() the unit tests were running slower than expected in some situations. In this particular situation http_do_bio() is going to be called at the end of the transport_connect_cb() (initially), or http_readcb(), sending all of the scheduled requests at once. This change affects only the test suite because it is the only place in the codebase where isc__nm_http_request() is used in order to ensure that the server is able to handle multiple HTTP/2 streams at once. --- lib/isc/netmgr/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 4e77a2ba82..11eefc165c 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1625,6 +1625,7 @@ isc__nm_http_request(isc_nmhandle_t *handle, isc_region_t *region, REQUIRE(VALID_NMHANDLE(handle)); REQUIRE(VALID_NMSOCK(handle->sock)); REQUIRE(handle->sock->tid == isc_nm_tid()); + REQUIRE(atomic_load(&handle->sock->client)); REQUIRE(cb != NULL); @@ -1640,7 +1641,6 @@ isc__nm_http_request(isc_nmhandle_t *handle, isc_region_t *region, goto error; } - http_do_bio(sock->h2.session, NULL, NULL, NULL); return (ISC_R_SUCCESS); error: From dbca22877ab2c6d98a2eacf963b66c097fc1fcfb Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 28 Jul 2021 19:30:30 +0300 Subject: [PATCH 08/15] Limit the number of requests sent per connection in DoH tests This commit ensures that only a limited number of requests is going to be sent over a single HTTP/2 connection. Before that change was introduced, it was possible to complete all of the planned sends via only one transport connection, which undermines the purpose of the tests using the quota facility. --- lib/isc/tests/doh_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index 198c7bfc23..e4f59ce7fd 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -725,7 +725,7 @@ doh_receive_send_reply_cb(isc_nmhandle_t *handle, isc_result_t eresult, int_fast64_t sends = atomic_fetch_sub(&nsends, 1); atomic_fetch_add(&csends, 1); atomic_fetch_add(&creads, 1); - if (sends > 0) { + if (sends > 0 && cbarg == NULL) { size_t i; for (i = 0; i < NWRITES / 2; i++) { eresult = isc__nm_http_request( @@ -733,7 +733,7 @@ doh_receive_send_reply_cb(isc_nmhandle_t *handle, isc_result_t eresult, &(isc_region_t){ .base = (uint8_t *)send_msg.base, .length = send_msg.len }, - doh_receive_send_reply_cb, cbarg); + doh_receive_send_reply_cb, (void *)1); if (eresult == ISC_R_CANCELED) { break; } From 5b52a7e37ed32e32fa671223b6d16e21f43436c4 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 29 Jul 2021 10:46:34 +0300 Subject: [PATCH 09/15] When terminating a client session, mark it as closing When an HTTP/2 client terminates a session it means that it is about to close the underlying connection. However, we were not doing that. As a result, with the latest changes to the test suite, which made it to limit amount of requests per a transport connection, the tests using quota would hang for quite a while. This commit fixes that. --- lib/isc/netmgr/http.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 11eefc165c..b5247f58b9 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -616,6 +616,9 @@ on_client_stream_close_callback(int32_t stream_id, if (rv != 0) { return (rv); } + /* Mark the session as closing one to finish it on a + * subsequent call to http_do_bio() */ + session->closing = true; } } else { return (NGHTTP2_ERR_CALLBACK_FAILURE); From a32faa20b43e463778794434b68bcfd8bf2bff8f Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 29 Jul 2021 11:43:29 +0300 Subject: [PATCH 10/15] DoH: replace a custom buffer code for POST data with isc_buffer_t This commit replaces the custom buffer code in client-side DoH code intended to keep track of POST data, with isc_buffer_t. --- lib/isc/netmgr/http.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index b5247f58b9..18d0427017 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -104,8 +104,7 @@ typedef struct http_cstream { int32_t stream_id; bool post; /* POST or GET */ - isc_region_t postdata; - size_t postdata_pos; + isc_buffer_t *postdata; char *GET_path; size_t GET_path_len; @@ -448,10 +447,12 @@ put_http_cstream(isc_mem_t *mctx, http_cstream_t *stream) { stream->GET_path = NULL; stream->GET_path_len = 0; } - if (stream->postdata.base != NULL) { - isc_mem_put(mctx, stream->postdata.base, - stream->postdata.length); + + if (stream->postdata != NULL) { + INSIST(stream->post); + isc_buffer_free(&stream->postdata); } + if (stream == stream->httpsock->h2.connect.cstream) { stream->httpsock->h2.connect.cstream = NULL; } @@ -863,17 +864,19 @@ client_read_callback(nghttp2_session *ngsession, int32_t stream_id, } if (cstream->post) { - size_t len = cstream->postdata.length - cstream->postdata_pos; + size_t len = isc_buffer_remaininglength(cstream->postdata); if (len > length) { len = length; } - memmove(buf, cstream->postdata.base + cstream->postdata_pos, - len); - cstream->postdata_pos += len; + if (len > 0) { + memmove(buf, isc_buffer_current(cstream->postdata), + len); + isc_buffer_forward(cstream->postdata, len); + } - if (cstream->postdata_pos == cstream->postdata.length) { + if (isc_buffer_remaininglength(cstream->postdata) == 0) { *data_flags |= NGHTTP2_DATA_FLAG_EOF; } @@ -898,7 +901,8 @@ client_submit_request(isc_nm_http_session_t *session, http_cstream_t *stream) { if (stream->post) { char p[64]; - snprintf(p, sizeof(p), "%u", stream->postdata.length); + snprintf(p, sizeof(p), "%u", + isc_buffer_usedlength(stream->postdata)); nghttp2_nv hdrs[] = { MAKE_NV2(":method", "POST"), MAKE_NV(":scheme", @@ -1559,12 +1563,9 @@ client_send(isc_nmhandle_t *handle, const isc_region_t *region) { if (cstream->post) { /* POST */ - cstream->postdata = (isc_region_t){ - .base = isc_mem_get(mctx, region->length), - .length = region->length - }; - memmove(cstream->postdata.base, region->base, region->length); - cstream->postdata_pos = 0; + isc_buffer_allocate(mctx, &cstream->postdata, region->length); + isc_buffer_putmem(cstream->postdata, region->base, + region->length); } else { /* GET */ size_t path_size = 0; From bd69c7c57c828139c6df8d02b194344ef91cf6f5 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 30 Jul 2021 13:02:41 +0300 Subject: [PATCH 11/15] Simplify buffering code logic in http_send_outgoing() This commit significantly simplifies the code in http_send_outgoing() as it was unnecessary complicated, because it was dealing with multiple statically and dynamically allocated buffers, making it extremely hard to follow, as well as making it to do unnecessary memory copying in some situations. This commit fixes these issues, while retaining the high level buffering logic. --- lib/isc/netmgr/http.c | 111 +++++++++++------------------------------- 1 file changed, 28 insertions(+), 83 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 18d0427017..802441a41c 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -164,9 +164,9 @@ typedef struct isc_http_send_req { isc_nm_http_session_t *session; isc_nmhandle_t *transphandle; isc_nmhandle_t *httphandle; - isc_region_t data; isc_nm_cb_t cb; void *cbarg; + isc_buffer_t *pending_write_data; isc__nm_http_pending_callbacks_t pending_write_callbacks; } isc_http_send_req_t; @@ -1029,7 +1029,7 @@ http_writecb(isc_nmhandle_t *handle, isc_result_t result, void *arg) { isc_nmhandle_detach(&req->httphandle); } - isc_mem_put(session->mctx, req->data.base, req->data.length); + isc_buffer_free(&req->pending_write_data); isc_mem_put(session->mctx, req, sizeof(*req)); session->sending--; @@ -1059,8 +1059,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc_nm_cb_t cb, void *cbarg) { isc_http_send_req_t *send = NULL; size_t total = 0; - uint8_t tmp_data[8192] = { 0 }; - uint8_t *prepared_data = &tmp_data[0]; + isc_region_t send_data = { 0 }; isc_nmhandle_t *transphandle = NULL; #ifdef ENABLE_HTTP_WRITE_BUFFERING size_t max_total_write_size = 0; @@ -1095,25 +1094,20 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, } /* reallocate buffer if required */ - if (new_total > sizeof(tmp_data)) { - uint8_t *old_prepared_data = prepared_data; - const bool allocated = prepared_data != tmp_data; - - prepared_data = isc_mem_get(session->mctx, new_total); - memmove(prepared_data, old_prepared_data, total); - if (allocated) { - isc_mem_put(session->mctx, old_prepared_data, - total); - } + if (session->pending_write_data == NULL) { + isc_buffer_allocate(session->mctx, + &session->pending_write_data, + INITIAL_DNS_MESSAGE_BUFFER_SIZE); + isc_buffer_setautorealloc(session->pending_write_data, + true); } - memmove(&prepared_data[total], data, pending); + isc_buffer_putmem(session->pending_write_data, data, pending); total = new_total; } #ifdef ENABLE_HTTP_WRITE_BUFFERING - max_total_write_size = total; if (session->pending_write_data != NULL) { - max_total_write_size += + max_total_write_size = isc_buffer_usedlength(session->pending_write_data); } @@ -1122,31 +1116,14 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, * within some tools (e.g. flamethrower). */ if (max_total_write_size >= FLUSH_HTTP_WRITE_BUFFER_AFTER) { /* Case 1: We have equal or more than - * FLUSH_HTTP_WRITE_BUFFER_AFTER bytes to send. Let's put the - * data which we have just obtained from nghttp2 into the - * pending write buffer and flush it. */ - - /* Let's allocate a new write buffer if there is none. */ - if (session->pending_write_data == NULL) { - isc_buffer_allocate(session->mctx, - &session->pending_write_data, - max_total_write_size); - } - - isc_buffer_putmem(session->pending_write_data, prepared_data, - total); - if (prepared_data != &tmp_data[0]) { - isc_mem_put(session->mctx, prepared_data, total); - } - + * FLUSH_HTTP_WRITE_BUFFER_AFTER bytes to send. Let's flush it. + */ total = max_total_write_size; - prepared_data = isc_buffer_base(session->pending_write_data); } else if (session->sending > 0 && total > 0) { /* Case 2: There is one or more write requests in flight and * we have some new data form nghttp2 to send. Let's put the * write callback (if any) into the pending write callbacks - * list and add the new data into the pending write - * buffer. Then let's return from the function: as soon as the + * list. Then let's return from the function: as soon as the * "in-flight" write callback get's called or we have reached * FLUSH_HTTP_WRITE_BUFFER_AFTER bytes in the write buffer, we * will flush the buffer. */ @@ -1161,21 +1138,7 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, ISC_LIST_APPEND(session->pending_write_callbacks, newcb, link); } - - if (session->pending_write_data == NULL) { - isc_buffer_allocate(session->mctx, - &session->pending_write_data, - total); - isc_buffer_setautorealloc(session->pending_write_data, - true); - } - - isc_buffer_putmem(session->pending_write_data, prepared_data, - total); - if (prepared_data != &tmp_data[0]) { - isc_mem_put(session->mctx, prepared_data, total); - } - goto failure; + goto nothing_to_send; } else if (session->sending == 0 && total == 0 && session->pending_write_data != NULL) { @@ -1185,10 +1148,8 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc_region_t region = { 0 }; total = isc_buffer_usedlength(session->pending_write_data); INSIST(total > 0); - INSIST(prepared_data == &tmp_data[0]); isc_buffer_usedregion(session->pending_write_data, ®ion); INSIST(total == region.length); - prepared_data = region.base; } else { /* The other cases are, uninteresting, fall-through ones. */ /* In the following cases (4-6) we will just bail out. */ @@ -1207,54 +1168,38 @@ http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, (total > 0 && session->sending == 0)); } #else - INSIST(session->pending_write_data == NULL); INSIST(ISC_LIST_EMPTY(session->pending_write_callbacks)); #endif /* ENABLE_HTTP_WRITE_BUFFERING */ if (total == 0) { - INSIST(prepared_data == &tmp_data[0]); /* No data returned */ - goto failure; + goto nothing_to_send; } + /* If we have reached the point it means that we need to send some + * data and flush the outgoing buffer. The code below does that. */ send = isc_mem_get(session->mctx, sizeof(*send)); - if (prepared_data == &tmp_data[0]) { - *send = (isc_http_send_req_t){ - .data.base = isc_mem_get(session->mctx, total), - .data.length = total, - }; - memmove(send->data.base, tmp_data, total); - } else if (session->pending_write_data != NULL) { - *send = (isc_http_send_req_t){ - .data.base = isc_mem_get(session->mctx, total), - .data.length = total, - }; - memmove(send->data.base, - isc_buffer_base(session->pending_write_data), total); - isc_buffer_free(&session->pending_write_data); - } else { - *send = (isc_http_send_req_t){ - .data.base = prepared_data, - .data.length = total, - }; - } + + *send = (isc_http_send_req_t){ .pending_write_data = + session->pending_write_data, + .cb = cb, + .cbarg = cbarg }; + session->pending_write_data = NULL; + move_pending_send_callbacks(session, send); send->transphandle = transphandle; isc__nm_httpsession_attach(session, &send->session); if (cb != NULL) { INSIST(VALID_NMHANDLE(httphandle)); - send->cb = cb; - send->cbarg = cbarg; isc_nmhandle_attach(httphandle, &send->httphandle); } - move_pending_send_callbacks(session, send); - session->sending++; - isc_nm_send(transphandle, &send->data, http_writecb, send); + isc_buffer_usedregion(send->pending_write_data, &send_data); + isc_nm_send(transphandle, &send_data, http_writecb, send); return (true); -failure: +nothing_to_send: isc_nmhandle_detach(&transphandle); return (false); } From e301e1e3b8eae1c3e4479fcaac3464dcb1923bc7 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 2 Aug 2021 14:43:54 +0300 Subject: [PATCH 12/15] Avoid memory copying during send in TLS stream At least at this point doing memory copying is not required. Probably it was a workaround for some problem in the earlier days of DoH, at this point it appears to be a waste of CPU cycles. --- lib/isc/netmgr/tlsstream.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index f203a94919..f5e65e930d 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -676,7 +676,6 @@ isc__nm_async_tlssend(isc__networker_t *worker, isc__netievent_t *ev0) { tls_do_bio(sock, NULL, req, false); done: - isc_mem_free(sock->mgr->mctx, req->uvbuf.base); isc__nm_uvreq_put(&req, sock); return; } @@ -704,9 +703,7 @@ isc__nm_tls_send(isc_nmhandle_t *handle, const isc_region_t *region, isc_nmhandle_attach(handle, &uvreq->handle); uvreq->cb.send = cb; uvreq->cbarg = cbarg; - - uvreq->uvbuf.base = isc_mem_allocate(sock->mgr->mctx, region->length); - memmove(uvreq->uvbuf.base, region->base, region->length); + uvreq->uvbuf.base = (char *)region->base; uvreq->uvbuf.len = region->length; /* From e639957b586a0824e32ef7db7f04cd1708fa8da7 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 2 Aug 2021 17:15:13 +0300 Subject: [PATCH 13/15] Optimise TLS stream for small write size (>= 512 bytes) This commit changes TLS stream behaviour in such a way, that it is now optimised for small writes. In the case there is a need to write less or equal to 512 bytes, we could avoid calling the memory allocator at the expense of possibly slight increase in memory usage. In case of larger writes, the behviour remains unchanged. --- lib/isc/netmgr/netmgr-int.h | 1 + lib/isc/netmgr/tlsstream.c | 25 ++++++++++++++++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index c75ddddfb8..23cfe8cbf1 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -754,6 +754,7 @@ typedef struct isc_nmsocket_tls_send_req { void *cbarg; isc_nmhandle_t *handle; bool finish; + uint8_t smallbuf[512]; } isc_nmsocket_tls_send_req_t; typedef enum isc_http_request_type { diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index f5e65e930d..0ccf68b4eb 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -123,8 +123,15 @@ tls_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) { } } - isc_mem_put(handle->sock->mgr->mctx, send_req->data.base, - send_req->data.length); + /* We are tying to avoid a memory allocation for small write + * requests. See the mirroring code in the tls_send_outgoing() + * function. */ + if (ISC_UNLIKELY(send_req->data.length > sizeof(send_req->smallbuf))) { + isc_mem_put(handle->sock->mgr->mctx, send_req->data.base, + send_req->data.length); + } else { + INSIST(&send_req->smallbuf[0] == send_req->data.base); + } isc_mem_put(handle->sock->mgr->mctx, send_req, sizeof(*send_req)); tlssock->tlsstream.nsending--; @@ -236,11 +243,15 @@ tls_send_outgoing(isc_nmsocket_t *sock, bool finish, isc_nmhandle_t *tlshandle, } send_req = isc_mem_get(sock->mgr->mctx, sizeof(*send_req)); - *send_req = (isc_nmsocket_tls_send_req_t){ - .finish = finish, - .data.base = isc_mem_get(sock->mgr->mctx, pending), - .data.length = pending - }; + *send_req = (isc_nmsocket_tls_send_req_t){ .finish = finish, + .data.length = pending }; + + /* Let's try to avoid a memory allocation for small write requests */ + if (ISC_UNLIKELY((size_t)pending > sizeof(send_req->smallbuf))) { + send_req->data.base = isc_mem_get(sock->mgr->mctx, pending); + } else { + send_req->data.base = &send_req->smallbuf[0]; + } isc__nmsocket_attach(sock, &send_req->tlssock); if (cb != NULL) { From d72b1fa5cd9bfb12d3d8aa844d6754cc49d92087 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 5 Aug 2021 12:42:40 +0300 Subject: [PATCH 14/15] Fix the doh_recv_send() logic in the doh_test The commit fixes the doh_recv_send() because occasionally it would fail because it did not wait for all responses to be sent, making the check for ssends value to nit pass. --- lib/isc/tests/doh_test.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index e4f59ce7fd..4f5c464e0a 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -1075,6 +1075,14 @@ doh_recv_send(void **state) { isc_thread_create(doh_connect_thread, connect_nm, &threads[i]); } + /* wait for the all responses from the server */ + while (atomic_load(&ssends) < atomic_load(&total_sends)) { + if (atomic_load(&was_error)) { + break; + } + isc_test_nap(100); + } + for (size_t i = 0; i < nthreads; i++) { isc_thread_join(threads[i], NULL); } From f85d899f55cc72f81b640ea88c60113d7fef2915 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 5 Aug 2021 16:01:04 +0300 Subject: [PATCH 15/15] Add a CHANGES entry for the crash fix [GL #2851] This commit adds a CHANGES entry for the fixed crash, caused by detaching from the session->handle too early when sending HTTP/2 session data. --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 64894bb7c2..0a9afeef7e 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5692. [bug] Fix a rare crash in the DoH code caused by + detaching from an HTTP/2 session handle too early when + sending data. [GL #2851] + 5691. [bug] 'rndc freeze' with in-view zones present would spuriously report failures. [GL #2844]