TLSDNS: call send callbacks after only the data was sent

This commit ensures that write callbacks are getting called only after
the data has been sent via the network.

Without this fix, a situation could appear when a write callback could
get called before the actual encrypted data would have been sent to
the network. Instead, it would get called right after it would have
been passed to the OpenSSL (i.e. encrypted).

Most likely, the issue does not reveal itself often because the
callback call was asynchronous, so in most cases it should have been
called after the data has been sent, but that was not guaranteed by
the code logic.

Also, this commit removes one memory allocation (netievent) from a hot
path, as there is no need to call this callback asynchronously
anymore.
This commit is contained in:
Artem Boldariev 2022-04-13 16:24:20 +03:00
parent 66080a6d91
commit 8b19f62ac5
3 changed files with 32 additions and 8 deletions

View file

@ -916,6 +916,7 @@ struct isc_nmsocket {
TLS_STATE_CLOSING
} state;
isc_region_t senddata;
ISC_LIST(isc__nm_uvreq_t) sendreqs;
bool cycle;
isc_result_t pending_error;
/* List of active send requests. */

View file

@ -1280,6 +1280,8 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) {
isc__nm_tls_cleanup_data(sock);
isc__nm_http_cleanup_data(sock);
#endif
INSIST(ISC_LIST_EMPTY(sock->tls.sendreqs));
#ifdef NETMGR_TRACE
LOCK(&sock->mgr->lock);
ISC_LIST_UNLINK(sock->mgr->active_sockets, sock, active_link);
@ -1468,6 +1470,8 @@ isc___nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type,
.inactivereqs = isc_astack_new(
mgr->mctx, ISC_NM_REQS_STACK_SIZE) };
ISC_LIST_INIT(sock->tls.sendreqs);
if (iface != NULL) {
family = iface->type.sa.sa_family;
sock->iface = *iface;

View file

@ -77,6 +77,9 @@ async_tlsdns_cycle(isc_nmsocket_t *sock) __attribute__((unused));
static isc_result_t
tls_cycle(isc_nmsocket_t *sock);
static void
call_pending_send_callbacks(isc_nmsocket_t *sock, const isc_result_t result);
static bool
can_log_tlsdns_quota(void) {
isc_stdtime_t now, last;
@ -820,6 +823,7 @@ isc__nm_tlsdns_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result,
}
destroy:
call_pending_send_callbacks(sock, result);
isc__nmsocket_prep_destroy(sock);
/*
@ -1149,7 +1153,19 @@ tls_error(isc_nmsocket_t *sock, isc_result_t result) {
}
static void
free_senddata(isc_nmsocket_t *sock) {
call_pending_send_callbacks(isc_nmsocket_t *sock, const isc_result_t result) {
isc__nm_uvreq_t *cbreq = ISC_LIST_HEAD(sock->tls.sendreqs);
while (cbreq != NULL) {
isc__nm_uvreq_t *next = ISC_LIST_NEXT(cbreq, link);
ISC_LIST_UNLINK(sock->tls.sendreqs, cbreq, link);
INSIST(sock == cbreq->handle->sock);
isc__nm_sendcb(sock, cbreq, result, false);
cbreq = next;
}
}
static void
free_senddata(isc_nmsocket_t *sock, const isc_result_t result) {
REQUIRE(VALID_NMSOCK(sock));
REQUIRE(sock->tls.senddata.base != NULL);
REQUIRE(sock->tls.senddata.length > 0);
@ -1158,23 +1174,26 @@ free_senddata(isc_nmsocket_t *sock) {
sock->tls.senddata.length);
sock->tls.senddata.base = NULL;
sock->tls.senddata.length = 0;
call_pending_send_callbacks(sock, result);
}
static void
tls_write_cb(uv_write_t *req, int status) {
isc_result_t result;
isc_result_t result = status != 0 ? isc__nm_uverr2result(status)
: ISC_R_SUCCESS;
isc__nm_uvreq_t *uvreq = (isc__nm_uvreq_t *)req->data;
isc_nmsocket_t *sock = uvreq->sock;
isc_nm_timer_stop(uvreq->timer);
isc_nm_timer_detach(&uvreq->timer);
free_senddata(sock);
free_senddata(sock, result);
isc__nm_uvreq_put(&uvreq, sock);
if (status != 0) {
tls_error(sock, isc__nm_uverr2result(status));
tls_error(sock, result);
return;
}
@ -1224,7 +1243,7 @@ tls_cycle_output(isc_nmsocket_t *sock) {
if (r == pending) {
/* Wrote everything, restart */
isc__nm_uvreq_put(&req, sock);
free_senddata(sock);
free_senddata(sock, ISC_R_SUCCESS);
continue;
}
@ -1239,7 +1258,7 @@ tls_cycle_output(isc_nmsocket_t *sock) {
} else {
result = isc__nm_uverr2result(r);
isc__nm_uvreq_put(&req, sock);
free_senddata(sock);
free_senddata(sock, result);
break;
}
@ -1248,7 +1267,7 @@ tls_cycle_output(isc_nmsocket_t *sock) {
if (r < 0) {
result = isc__nm_uverr2result(r);
isc__nm_uvreq_put(&req, sock);
free_senddata(sock);
free_senddata(sock, result);
break;
}
@ -1727,7 +1746,7 @@ tlsdns_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
/* SSL_write_ex() doesn't do partial writes */
INSIST(sendlen == bytes);
isc__nm_sendcb(sock, req, ISC_R_SUCCESS, true);
ISC_LIST_APPEND(sock->tls.sendreqs, req, link);
async_tlsdns_cycle(sock);
return (ISC_R_SUCCESS);
}