Commit f3d08cd4 authored by Ondřej Surý's avatar Ondřej Surý
Browse files

Merge branch '2221-netmgr-fixes-from-unit-testing' into 'main'

Fix the problems found when writing the unit test for netmgr

See merge request !4283
parents 417632eb 58a0e959
Pipeline #54918 passed with stages
in 2 minutes and 8 seconds
5520. [bug] Fixed a number of shutdown races, reference counting
errors, and spurious log messages that could occur
in the network manager. [GL #2221]
5519. [cleanup] Unused source code was removed: lib/dns/dbtable.c,
lib/dns/portlist.c, lib/isc/bufferlist.c, and code
related to those files. [GL #2060]
......
......@@ -183,8 +183,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly,
/* Do not flush master files */
named_server_flushonshutdown(named_g_server, false);
named_os_shutdownmsg(cmdline, *text);
isc_app_shutdown();
result = ISC_R_SUCCESS;
result = ISC_R_SHUTTINGDOWN;
} else if (command_compare(command, NAMED_COMMAND_STOP)) {
/*
* "stop" is the same as "halt" except it does
......@@ -201,8 +200,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly,
#endif /* ifdef HAVE_LIBSCF */
named_server_flushonshutdown(named_g_server, true);
named_os_shutdownmsg(cmdline, *text);
isc_app_shutdown();
result = ISC_R_SUCCESS;
result = ISC_R_SHUTTINGDOWN;
} else if (command_compare(command, NAMED_COMMAND_ADDZONE) ||
command_compare(command, NAMED_COMMAND_MODZONE))
{
......
......@@ -14,6 +14,7 @@
#include <inttypes.h>
#include <stdbool.h>
#include <isc/app.h>
#include <isc/base64.h>
#include <isc/buffer.h>
#include <isc/event.h>
......@@ -225,6 +226,11 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
conn->sending = false;
if (conn->result == ISC_R_SHUTTINGDOWN) {
isc_app_shutdown();
goto cleanup_sendhandle;
}
if (atomic_load_acquire(&listener->controls->shuttingdown) ||
result == ISC_R_CANCELED)
{
......@@ -245,14 +251,7 @@ control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
isc_nmhandle_detach(&conn->sendhandle);
result = isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage,
conn);
if (result != ISC_R_SUCCESS) {
conn->reading = false;
isc_nmhandle_detach(&conn->readhandle);
return;
}
isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn);
return;
cleanup_sendhandle:
......@@ -291,12 +290,12 @@ conn_cleanup(controlconnection_t *conn) {
}
static void
control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
controlconnection_t *conn = (controlconnection_t *)arg;
control_respond(isc_nmhandle_t *handle, controlconnection_t *conn) {
controllistener_t *listener = conn->listener;
isccc_sexpr_t *data = NULL;
isc_buffer_t b;
isc_region_t r;
isc_result_t result;
result = isccc_cc_createresponse(conn->request, conn->now,
conn->now + 60, &conn->response);
......@@ -304,17 +303,22 @@ control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
goto cleanup;
}
if (conn->result == ISC_R_SHUTTINGDOWN) {
result = ISC_R_SUCCESS;
} else {
result = conn->result;
}
data = isccc_alist_lookup(conn->response, "_data");
if (data != NULL) {
if (isccc_cc_defineuint32(data, "result", conn->result) == NULL)
{
if (isccc_cc_defineuint32(data, "result", result) == NULL) {
goto cleanup;
}
}
if (conn->result != ISC_R_SUCCESS) {
if (result != ISC_R_SUCCESS) {
if (data != NULL) {
const char *estr = isc_result_totext(conn->result);
const char *estr = isc_result_totext(result);
if (isccc_cc_definestring(data, "err", estr) == NULL) {
goto cleanup;
}
......@@ -363,11 +367,7 @@ control_respond(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
isc_nmhandle_detach(&conn->cmdhandle);
result = isc_nm_send(conn->sendhandle, &r, control_senddone, conn);
if (result != ISC_R_SUCCESS) {
isc_nmhandle_detach(&conn->sendhandle);
conn->sending = false;
}
isc_nm_send(conn->sendhandle, &r, control_senddone, conn);
return;
......@@ -391,7 +391,7 @@ control_command(isc_task_t *task, isc_event_t *event) {
conn->result = named_control_docommand(conn->request,
listener->readonly, &conn->text);
control_respond(conn->cmdhandle, conn->result, conn);
control_respond(conn->cmdhandle, conn);
done:
isc_event_free(&event);
......@@ -532,7 +532,7 @@ control_recvmessage(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
isc_nonce_buf(&conn->nonce, sizeof(conn->nonce));
}
conn->result = ISC_R_SUCCESS;
control_respond(handle, result, conn);
control_respond(handle, conn);
return;
}
......@@ -596,10 +596,9 @@ conn_put(void *arg) {
maybe_free_listener(listener);
}
static isc_result_t
static void
newconnection(controllistener_t *listener, isc_nmhandle_t *handle) {
controlconnection_t *conn = NULL;
isc_result_t result;
conn = isc_nmhandle_getdata(handle);
if (conn == NULL) {
......@@ -627,26 +626,7 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) {
isc_nmhandle_attach(handle, &conn->readhandle);
conn->reading = true;
result = isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage,
conn);
if (result != ISC_R_SUCCESS) {
isc_nmhandle_detach(&conn->readhandle);
conn->reading = false;
goto cleanup;
}
return (ISC_R_SUCCESS);
cleanup:
/*
* conn_reset() handles the cleanup.
*/
#ifdef ENABLE_AFL
if (named_g_fuzz_type == isc_fuzz_rndc) {
named_fuzz_notify();
}
#endif /* ifdef ENABLE_AFL */
return (result);
isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn);
}
static isc_result_t
......@@ -672,17 +652,7 @@ control_newconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
return (ISC_R_FAILURE);
}
result = newconnection(listener, handle);
if (result != ISC_R_SUCCESS) {
char socktext[ISC_SOCKADDR_FORMATSIZE];
isc_sockaddr_format(&peeraddr, socktext, sizeof(socktext));
isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
NAMED_LOGMODULE_CONTROL, ISC_LOG_WARNING,
"dropped command channel from %s: %s", socktext,
isc_result_totext(result));
return (result);
}
newconnection(listener, handle);
return (ISC_R_SUCCESS);
}
......
......@@ -402,13 +402,14 @@ static void
rndc_recvnonce(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
isccc_ccmsg_t *ccmsg = (isccc_ccmsg_t *)arg;
isccc_sexpr_t *response = NULL;
isccc_sexpr_t *_ctrl;
isc_nmhandle_t *sendhandle = NULL;
isccc_sexpr_t *_ctrl = NULL;
isccc_region_t source;
uint32_t nonce;
isccc_sexpr_t *request = NULL;
isccc_time_t now;
isc_region_t r;
isccc_sexpr_t *data;
isccc_sexpr_t *data = NULL;
isc_buffer_t b;
REQUIRE(ccmsg != NULL);
......@@ -484,13 +485,11 @@ rndc_recvnonce(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
isc_nmhandle_attach(handle, &recvdone_handle);
atomic_fetch_add_relaxed(&recvs, 1);
DO("schedule recv",
isccc_ccmsg_readmessage(ccmsg, rndc_recvdone, ccmsg));
isccc_ccmsg_readmessage(ccmsg, rndc_recvdone, ccmsg);
isc_nmhandle_t *sendhandle = NULL;
isc_nmhandle_attach(handle, &sendhandle);
atomic_fetch_add_relaxed(&sends, 1);
DO("send message", isc_nm_send(handle, &r, rndc_senddone, sendhandle));
isc_nm_send(handle, &r, rndc_senddone, sendhandle);
REQUIRE(recvnonce_handle == handle);
isc_nmhandle_detach(&recvnonce_handle);
......@@ -506,11 +505,12 @@ rndc_connected(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
isccc_ccmsg_t *ccmsg = (isccc_ccmsg_t *)arg;
char socktext[ISC_SOCKADDR_FORMATSIZE];
isccc_sexpr_t *request = NULL;
isccc_sexpr_t *data;
isccc_sexpr_t *data = NULL;
isccc_time_t now;
isc_region_t r;
isc_buffer_t b;
isc_nmhandle_t *connhandle = NULL;
isc_nmhandle_t *sendhandle = NULL;
REQUIRE(ccmsg != NULL);
......@@ -560,13 +560,11 @@ rndc_connected(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
isc_nmhandle_attach(handle, &recvnonce_handle);
atomic_fetch_add_relaxed(&recvs, 1);
DO("schedule recv",
isccc_ccmsg_readmessage(ccmsg, rndc_recvnonce, ccmsg));
isccc_ccmsg_readmessage(ccmsg, rndc_recvnonce, ccmsg);
isc_nmhandle_t *sendhandle = NULL;
isc_nmhandle_attach(handle, &sendhandle);
atomic_fetch_add_relaxed(&sends, 1);
DO("send message", isc_nm_send(handle, &r, rndc_senddone, sendhandle));
isc_nm_send(handle, &r, rndc_senddone, sendhandle);
isc_nmhandle_detach(&connhandle);
atomic_fetch_sub_release(&connects, 1);
......
......@@ -623,9 +623,8 @@ httpd_put(void *arg) {
#endif /* ENABLE_AFL */
}
static isc_result_t
static void
new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) {
isc_result_t result;
isc_httpd_t *httpd = NULL;
char *headerdata = NULL;
......@@ -668,12 +667,7 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) {
UNLOCK(&httpdmgr->lock);
isc_nmhandle_attach(handle, &httpd->readhandle);
result = isc_nm_read(handle, httpd_request, httpdmgr);
if (result != ISC_R_SUCCESS) {
isc_nmhandle_detach(&httpd->readhandle);
}
return (result);
isc_nm_read(handle, httpd_request, httpdmgr);
}
static isc_result_t
......@@ -699,7 +693,9 @@ httpd_newconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
return (ISC_R_FAILURE);
}
return (new_httpd(httpdmgr, handle));
new_httpd(httpdmgr, handle);
return (ISC_R_SUCCESS);
}
static isc_result_t
......@@ -963,13 +959,7 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
httpd->state = SEND;
isc_nmhandle_attach(handle, &httpd->sendhandle);
result = isc_nm_send(handle, &r, httpd_senddone, httpd);
if (result != ISC_R_SUCCESS) {
isc_nmhandle_detach(&httpd->sendhandle);
httpd->state = RECV;
isc_nm_resumeread(handle);
}
isc_nm_send(handle, &r, httpd_senddone, httpd);
return;
cleanup_readhandle:
......@@ -1137,12 +1127,12 @@ httpd_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
}
}
isc_nmhandle_detach(&httpd->sendhandle);
if (result != ISC_R_SUCCESS) {
return;
}
isc_nmhandle_detach(&httpd->sendhandle);
httpd->state = RECV;
isc_nm_resumeread(handle);
}
......
......@@ -200,7 +200,7 @@ isc_nm_resume(isc_nm_t *mgr);
* workers to resume.
*/
isc_result_t
void
isc_nm_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg);
/*
* Begin (or continue) reading on the socket associated with 'handle', and
......@@ -208,7 +208,7 @@ isc_nm_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg);
* is data to process.
*/
isc_result_t
void
isc_nm_pauseread(isc_nmhandle_t *handle);
/*%<
* Pause reading on this handle's socket, but remember the callback.
......@@ -228,7 +228,7 @@ isc_nm_cancelread(isc_nmhandle_t *handle);
* \li ...for which a read/recv callback has been defined.
*/
isc_result_t
void
isc_nm_resumeread(isc_nmhandle_t *handle);
/*%<
* Resume reading on the handle's socket.
......@@ -238,7 +238,7 @@ isc_nm_resumeread(isc_nmhandle_t *handle);
* \li ...for a socket with a defined read/recv callback.
*/
isc_result_t
void
isc_nm_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
void *cbarg);
/*%<
......
......@@ -660,6 +660,18 @@ isc__nmsocket_active(isc_nmsocket_t *sock);
* or, for child sockets, 'sock->parent->active'.
*/
bool
isc__nmsocket_deactivate(isc_nmsocket_t *sock);
/*%<
* @brief Deactivate active socket
*
* Atomically deactive the socket by setting @p sock->active or, for child
* sockets, @p sock->parent->active to @c false
*
* @param[in] sock - valid nmsocket
* @return @c false if the socket was already inactive, @c true otherwise
*/
void
isc__nmsocket_clearcb(isc_nmsocket_t *sock);
/*%<
......@@ -679,7 +691,7 @@ isc__nm_async_shutdown(isc__networker_t *worker, isc__netievent_t *ev0);
* close on them.
*/
isc_result_t
void
isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
void *cbarg);
/*%<
......@@ -700,14 +712,14 @@ isc__nm_async_udpsend(isc__networker_t *worker, isc__netievent_t *ev0);
* Callback handlers for asynchronous UDP events (listen, stoplisten, send).
*/
isc_result_t
void
isc__nm_tcp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
void *cbarg);
/*%<
* Back-end implementation of isc_nm_send() for TCP handles.
*/
isc_result_t
void
isc__nm_tcp_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg);
/*
* Back-end implementation of isc_nm_read() for TCP handles.
......@@ -718,13 +730,13 @@ isc__nm_tcp_close(isc_nmsocket_t *sock);
/*%<
* Close a TCP socket.
*/
isc_result_t
void
isc__nm_tcp_pauseread(isc_nmsocket_t *sock);
/*%<
* Pause reading on this socket, while still remembering the callback.
*/
isc_result_t
void
isc__nm_tcp_resumeread(isc_nmsocket_t *sock);
/*%<
* Resume reading from socket.
......@@ -774,7 +786,7 @@ isc__nm_async_tcpclose(isc__networker_t *worker, isc__netievent_t *ev0);
* stoplisten, send, read, pause, close).
*/
isc_result_t
void
isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
isc_nm_cb_t cb, void *cbarg);
/*%<
......
......@@ -429,6 +429,9 @@ isc_nm_destroy(isc_nm_t **mgr0) {
#endif /* ifdef WIN32 */
}
#ifdef NETMGR_TRACE
isc__nm_dump_active(mgr);
#endif
INSIST(references == 1);
/*
......@@ -728,6 +731,19 @@ isc__nmsocket_active(isc_nmsocket_t *sock) {
return (atomic_load(&sock->active));
}
bool
isc__nmsocket_deactivate(isc_nmsocket_t *sock) {
REQUIRE(VALID_NMSOCK(sock));
if (sock->parent != NULL) {
return (atomic_compare_exchange_strong(&sock->parent->active,
&(bool){ true }, false));
}
return (atomic_compare_exchange_strong(&sock->active, &(bool){ true },
false));
}
void
isc__nmsocket_attach(isc_nmsocket_t *sock, isc_nmsocket_t **target) {
REQUIRE(VALID_NMSOCK(sock));
......@@ -1377,7 +1393,7 @@ isc__nm_uvreq_get(isc_nm_t *mgr, isc_nmsocket_t *sock) {
REQUIRE(VALID_NM(mgr));
REQUIRE(VALID_NMSOCK(sock));
if (sock != NULL && atomic_load(&sock->active)) {
if (sock != NULL && isc__nmsocket_active(sock)) {
/* Try to reuse one */
req = isc_astack_pop(sock->inactivereqs);
}
......@@ -1416,7 +1432,7 @@ isc__nm_uvreq_put(isc__nm_uvreq_t **req0, isc_nmsocket_t *sock) {
handle = req->handle;
req->handle = NULL;
if (!atomic_load(&sock->active) ||
if (!isc__nmsocket_active(sock) ||
!isc_astack_trypush(sock->inactivereqs, req)) {
isc_mempool_put(sock->mgr->reqpool, req);
}
......@@ -1428,7 +1444,7 @@ isc__nm_uvreq_put(isc__nm_uvreq_t **req0, isc_nmsocket_t *sock) {
isc__nmsocket_detach(&sock);
}
isc_result_t
void
isc_nm_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
void *cbarg) {
REQUIRE(VALID_NMHANDLE(handle));
......@@ -1436,24 +1452,28 @@ isc_nm_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
switch (handle->sock->type) {
case isc_nm_udpsocket:
case isc_nm_udplistener:
return (isc__nm_udp_send(handle, region, cb, cbarg));
isc__nm_udp_send(handle, region, cb, cbarg);
break;
case isc_nm_tcpsocket:
return (isc__nm_tcp_send(handle, region, cb, cbarg));
isc__nm_tcp_send(handle, region, cb, cbarg);
break;
case isc_nm_tcpdnssocket:
return (isc__nm_tcpdns_send(handle, region, cb, cbarg));
isc__nm_tcpdns_send(handle, region, cb, cbarg);
break;
default:
INSIST(0);
ISC_UNREACHABLE();
}
}
isc_result_t
void
isc_nm_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
REQUIRE(VALID_NMHANDLE(handle));
switch (handle->sock->type) {
case isc_nm_tcpsocket:
return (isc__nm_tcp_read(handle, cb, cbarg));
isc__nm_tcp_read(handle, cb, cbarg);
break;
default:
INSIST(0);
ISC_UNREACHABLE();
......@@ -1474,7 +1494,7 @@ isc_nm_cancelread(isc_nmhandle_t *handle) {
}
}
isc_result_t
void
isc_nm_pauseread(isc_nmhandle_t *handle) {
REQUIRE(VALID_NMHANDLE(handle));
......@@ -1482,14 +1502,15 @@ isc_nm_pauseread(isc_nmhandle_t *handle) {
switch (sock->type) {
case isc_nm_tcpsocket:
return (isc__nm_tcp_pauseread(sock));
isc__nm_tcp_pauseread(sock);
break;
default:
INSIST(0);
ISC_UNREACHABLE();
}
}
isc_result_t
void
isc_nm_resumeread(isc_nmhandle_t *handle) {
REQUIRE(VALID_NMHANDLE(handle));
......@@ -1497,7 +1518,8 @@ isc_nm_resumeread(isc_nmhandle_t *handle) {
switch (sock->type) {
case isc_nm_tcpsocket:
return (isc__nm_tcp_resumeread(sock));
isc__nm_tcp_resumeread(sock);
break;
default:
INSIST(0);
ISC_UNREACHABLE();
......@@ -1838,7 +1860,8 @@ nmhandle_dump(isc_nmhandle_t *handle) {
static void
nmsocket_dump(isc_nmsocket_t *sock) {
isc_nmhandle_t *handle;
isc_nmhandle_t *handle = NULL;
LOCK(&sock->lock);
fprintf(stderr, "\n=================\n");
fprintf(stderr, "Active socket %p, type %s, refs %lu\n", sock,
......@@ -1850,25 +1873,37 @@ nmsocket_dump(isc_nmsocket_t *sock) {
backtrace_symbols_fd(sock->backtrace, sock->backtrace_size,
STDERR_FILENO);
fprintf(stderr, "\n");
fprintf(stderr, "Active handles:\n");
for (handle = ISC_LIST_HEAD(sock->active_handles); handle != NULL;
handle = ISC_LIST_NEXT(handle, active_link))
{
static bool first = true;
if (first) {
fprintf(stderr, "Active handles:\n");
first = false;
}
nmhandle_dump(handle);
}
fprintf(stderr, "\n");
UNLOCK(&sock->lock);
}
void
isc__nm_dump_active(isc_nm_t *nm) {
isc_nmsocket_t *sock;
isc_nmsocket_t *sock = NULL;
REQUIRE(VALID_NM(nm));
LOCK(&nm->lock);
fprintf(stderr, "Outstanding sockets\n");
for (sock = ISC_LIST_HEAD(nm->active_sockets); sock != NULL;
sock = ISC_LIST_NEXT(sock, active_link))
{
static bool first = true;
if (first) {
fprintf(stderr, "Outstanding sockets\n");
first = false;
}
nmsocket_dump(sock);
}
UNLOCK(&nm->lock);
......
......@@ -16,6 +16,7 @@
#include <isc/atomic.h>
#include <isc/buffer.h>
#include <isc/condition.h>
#include <isc/errno.h>
#include <isc/log.h>
#include <isc/magic.h>
#include <isc/mem.h>
......@@ -134,12 +135,15 @@ isc__nm_async_tcpconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
if (r != 0) {
/* We need to issue callbacks ourselves */
tcp_connect_cb(&req->uv_req.connect, r);
goto done;
LOCK(&sock->lock);
SIGNAL(&sock->cond);
UNLOCK(&sock->lock);
isc__nmsocket_detach(&sock);
return;
}
atomic_store(&sock->connected, true);
done:
LOCK(&sock->lock);
SIGNAL(&sock->cond);
UNLOCK(&sock->lock);
......@@ -160,6 +164,7 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) {
if (status != 0) {
req->cb.connect(NULL, isc__nm_uverr2result(status), req->cbarg);
isc__nm_uvreq_put(&req, sock);
isc__nmsocket_detach(&sock);