Commit 726cddb5 authored by Mark Andrews's avatar Mark Andrews
Browse files

4454. [bug] 'rndc dnstap -reopen' had a race issue. [RT #43089]

parent f431bf02
--- 9.11.0rc1 released ---
4454. [bug] 'rndc dnstap -reopen' had a race issue. [RT #43089]
4453. [bug] Prefetching of DS records failed to update their
RRSIGs. [RT #42865]
......
......@@ -3070,7 +3070,7 @@ configure_dnstap(const cfg_obj_t **maps, dns_view_t *view) {
fstrm_iothr_options_set_reopen_interval(fopt, i);
}
CHECKM(dns_dt_create(ns_g_mctx, dmode, dpath, fopt,
CHECKM(dns_dt_create(ns_g_mctx, dmode, dpath, &fopt,
&ns_g_server->dtenv),
"unable to create dnstap environment");
}
......@@ -13372,6 +13372,9 @@ isc_result_t
ns_server_dnstap(ns_server_t *server, isc_lex_t *lex, isc_buffer_t **text) {
#if HAVE_DNSTAP
char *ptr;
isc_result_t result;
isc_boolean_t reopen = ISC_FALSE;
int backups = 0;
if (server->dtenv == NULL)
return (ISC_R_NOTFOUND);
......@@ -13382,18 +13385,17 @@ ns_server_dnstap(ns_server_t *server, isc_lex_t *lex, isc_buffer_t **text) {
return (ISC_R_UNEXPECTEDEND);
/* "dnstap-reopen" was used in 9.11.0b1 */
if (strcasecmp(ptr, "dnstap-reopen") == 0)
return (dns_dt_reopen(server->dtenv, ISC_FALSE));
/* Find out what we are to do. */
ptr = next_token(lex, text);
if (ptr == NULL)
return (ISC_R_UNEXPECTEDEND);
if (strcasecmp(ptr, "dnstap-reopen") == 0) {
reopen = ISC_TRUE;
} else {
ptr = next_token(lex, text);
if (ptr == NULL)
return (ISC_R_UNEXPECTEDEND);
}
if (strcasecmp(ptr, "-reopen") == 0)
return (dns_dt_reopen(server->dtenv, -1));
else if ((strcasecmp(ptr, "-roll") == 0)) {
int backups = 0;
if (reopen || strcasecmp(ptr, "-reopen") == 0) {
backups = -1;
} else if ((strcasecmp(ptr, "-roll") == 0)) {
unsigned int n;
ptr = next_token(lex, text);
if (ptr != NULL) {
......@@ -13401,9 +13403,14 @@ ns_server_dnstap(ns_server_t *server, isc_lex_t *lex, isc_buffer_t **text) {
if (n != 1U)
return (ISC_R_BADNUMBER);
}
return (dns_dt_reopen(server->dtenv, backups));
} else
return (DNS_R_SYNTAX);
result = isc_task_beginexclusive(server->task);
RUNTIME_CHECK(result == ISC_R_SUCCESS);
result = dns_dt_reopen(server->dtenv, backups);
isc_task_endexclusive(server->task);
return (result);
#else
UNUSED(server);
UNUSED(lex);
......
......@@ -13,3 +13,4 @@ rm -f dig.out*
rm -f ns*/named.lock
rm -f ns*/dnstap.out
rm -f ns*/dnstap.out.save
rm -f ns*/dnstap.out.save.?
......@@ -38,14 +38,11 @@ $DIG +short @10.53.0.3 -p 5300 a.example > dig.out
mv ns1/dnstap.out ns1/dnstap.out.save
mv ns2/dnstap.out ns2/dnstap.out.save
sleep 2
$RNDCCMD -s 10.53.0.1 dnstap-reopen | sed 's/^/I:ns1 /'
$RNDCCMD -s 10.53.0.2 dnstap -reopen | sed 's/^/I:ns2 /'
$RNDCCMD -s 10.53.0.3 dnstap -roll | sed 's/^/I:ns3 /'
$DIG +short @10.53.0.3 -p 5300 a.example > dig.out
sleep 1
# XXX: file output should be flushed once a second according
# to the libfstrm source, but it doesn't seem to happen until
......@@ -57,6 +54,8 @@ $RNDCCMD -s 10.53.0.2 stop | sed 's/^/I:ns2 /'
$RNDCCMD -s 10.53.0.3 stop | sed 's/^/I:ns3 /'
sleep 1
echo "I:checking initial message counts"
udp1=`$DNSTAPREAD ns1/dnstap.out.save | grep "UDP " | wc -l`
tcp1=`$DNSTAPREAD ns1/dnstap.out.save | grep "TCP " | wc -l`
aq1=`$DNSTAPREAD ns1/dnstap.out.save | grep "AQ " | wc -l`
......@@ -85,7 +84,6 @@ cr3=`$DNSTAPREAD ns3/dnstap.out.save | grep "CR " | wc -l`
rq3=`$DNSTAPREAD ns3/dnstap.out.save | grep "RQ " | wc -l`
rr3=`$DNSTAPREAD ns3/dnstap.out.save | grep "RR " | wc -l`
echo "I:checking initial message counts"
echo "I: checking UDP message counts"
ret=0
[ $udp1 -eq 0 ] || {
......@@ -198,6 +196,8 @@ ret=0
if [ $ret != 0 ]; then echo "I: failed"; fi
status=`expr $status + $ret`
echo "I:checking reopened message counts"
udp1=`$DNSTAPREAD ns1/dnstap.out | grep "UDP " | wc -l`
tcp1=`$DNSTAPREAD ns1/dnstap.out | grep "TCP " | wc -l`
aq1=`$DNSTAPREAD ns1/dnstap.out | grep "AQ " | wc -l`
......@@ -225,8 +225,6 @@ cr3=`$DNSTAPREAD ns3/dnstap.out | grep "CR " | wc -l`
rq3=`$DNSTAPREAD ns3/dnstap.out | grep "RQ " | wc -l`
rr3=`$DNSTAPREAD ns3/dnstap.out | grep "RR " | wc -l`
echo "I:checking reopened message counts"
echo "I: checking UDP message counts"
ret=0
[ $udp1 -eq 0 ] || {
......
......@@ -198,7 +198,7 @@ main(int argc, char *argv[]) {
isc_buffer_t *b = NULL;
dns_dtdata_t *dt = NULL;
const dns_master_style_t *style = &dns_master_style_debug;
dns_dthandle_t handle = {dns_dtmode_none, NULL};
dns_dthandle_t *handle = NULL;
int rv = 0, ch;
while ((ch = isc_commandline_parse(argc, argv, "mpy")) != -1) {
......@@ -230,7 +230,9 @@ main(int argc, char *argv[]) {
RUNTIME_CHECK(isc_mem_create(0, 0, &mctx) == ISC_R_SUCCESS);
CHECKM(dns_dt_open(argv[0], dns_dtmode_file, &handle),
dns_result_register();
CHECKM(dns_dt_open(argv[0], dns_dtmode_file, mctx, &handle),
"dns_dt_openfile");
for (;;) {
......@@ -238,7 +240,7 @@ main(int argc, char *argv[]) {
isc_uint8_t *data;
size_t datalen;
result = dns_dt_getframe(&handle, &data, &datalen);
result = dns_dt_getframe(handle, &data, &datalen);
if (result == ISC_R_NOMORE)
break;
else
......@@ -303,7 +305,8 @@ main(int argc, char *argv[]) {
cleanup:
if (dt != NULL)
dns_dtdata_free(&dt);
dns_dt_close(&handle);
if (handle != NULL)
dns_dt_close(&handle);
if (message != NULL)
dns_message_destroy(&message);
if (b != NULL)
......
......@@ -85,6 +85,12 @@ struct dns_dtmsg {
Dnstap__Message m;
};
struct dns_dthandle {
dns_dtmode_t mode;
struct fstrm_reader *reader;
isc_mem_t *mctx;
};
struct dns_dtenv {
unsigned int magic;
isc_refcount_t refcount;
......@@ -92,7 +98,7 @@ struct dns_dtenv {
isc_mem_t *mctx;
struct fstrm_iothr *iothr;
struct fstrm_writer *fw;
struct fstrm_iothr_options *fopt;
isc_region_t identity;
isc_region_t version;
......@@ -113,6 +119,11 @@ static isc_thread_key_t dt_key;
static isc_once_t mutex_once = ISC_ONCE_INIT;
static isc_mem_t *dt_mctx = NULL;
/*
* Change under task exclusive.
*/
static unsigned int ioqversion;
static void
mutex_init(void) {
RUNTIME_CHECK(isc_mutex_init(&dt_mutex) == ISC_R_SUCCESS);
......@@ -120,7 +131,7 @@ mutex_init(void) {
static void
dtfree(void *arg) {
UNUSED(arg);
free(arg);
isc_thread_key_setspecific(dt_key, NULL);
}
......@@ -160,7 +171,7 @@ unlock:
isc_result_t
dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path,
struct fstrm_iothr_options *fopt, dns_dtenv_t **envp)
struct fstrm_iothr_options **foptp, dns_dtenv_t **envp)
{
isc_result_t result = ISC_R_SUCCESS;
fstrm_res res;
......@@ -172,12 +183,14 @@ dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path,
REQUIRE(path != NULL);
REQUIRE(envp != NULL && *envp == NULL);
REQUIRE(fopt != NULL);
REQUIRE(foptp!= NULL && *foptp != NULL);
isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSTAP,
DNS_LOGMODULE_DNSTAP, ISC_LOG_INFO,
"opening dnstap destination '%s'", path);
ioqversion++;
env = isc_mem_get(mctx, sizeof(dns_dtenv_t));
if (env == NULL)
CHECK(ISC_R_NOMEMORY);
......@@ -200,7 +213,6 @@ dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path,
if (res != fstrm_res_success)
CHECK(ISC_R_FAILURE);
if (mode == dns_dtmode_file) {
ffwopt = fstrm_file_options_init();
if (ffwopt != NULL) {
......@@ -220,20 +232,17 @@ dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path,
if (fw == NULL)
CHECK(ISC_R_FAILURE);
/*
* Remember 'fw' so we can close and reopen it later.
*/
env->fw = fw;
env->iothr = fstrm_iothr_init(fopt, &fw);
env->iothr = fstrm_iothr_init(*foptp, &fw);
if (env->iothr == NULL) {
isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSTAP,
DNS_LOGMODULE_DNSTAP, ISC_LOG_WARNING,
"unable to initialize dnstap I/O thread");
env->fw = NULL;
fstrm_writer_destroy(&fw);
CHECK(ISC_R_FAILURE);
}
env->mode = mode;
env->fopt = *foptp;
*foptp = NULL;
isc_mem_attach(mctx, &env->mctx);
......@@ -267,26 +276,63 @@ dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path,
isc_result_t
dns_dt_reopen(dns_dtenv_t *env, int roll) {
isc_result_t result;
isc_logfile_t file;
isc_result_t result = ISC_R_SUCCESS;
fstrm_res res;
isc_logfile_t file;
struct fstrm_unix_writer_options *fuwopt = NULL;
struct fstrm_file_options *ffwopt = NULL;
struct fstrm_writer_options *fwopt = NULL;
struct fstrm_writer *fw = NULL;
REQUIRE(VALID_DTENV(env));
if (env->fw == NULL)
return (ISC_R_FAILURE);
/*
* Check that we can create a new fw object.
*/
fwopt = fstrm_writer_options_init();
if (fwopt == NULL)
return (ISC_R_NOMEMORY);
res = fstrm_writer_options_add_content_type(fwopt,
DNSTAP_CONTENT_TYPE,
sizeof(DNSTAP_CONTENT_TYPE) - 1);
if (res != fstrm_res_success)
CHECK(ISC_R_FAILURE);
if (env->mode == dns_dtmode_file) {
ffwopt = fstrm_file_options_init();
if (ffwopt != NULL) {
fstrm_file_options_set_file_path(ffwopt, env->path);
fw = fstrm_file_writer_init(ffwopt, fwopt);
}
} else if (env->mode == dns_dtmode_unix) {
fuwopt = fstrm_unix_writer_options_init();
if (fuwopt != NULL) {
fstrm_unix_writer_options_set_socket_path(fuwopt,
env->path);
fw = fstrm_unix_writer_init(fuwopt, fwopt);
}
} else
CHECK(ISC_R_NOTIMPLEMENTED);
if (fw == NULL)
CHECK(ISC_R_FAILURE);
/*
* We are committed here.
*/
isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSTAP,
DNS_LOGMODULE_DNSTAP, ISC_LOG_INFO,
"%s dnstap destination '%s'",
(roll < 0) ? "reopening" : "rolling",
env->path);
if (env->iothr != NULL)
fstrm_iothr_destroy(&env->iothr);
res = fstrm_writer_close(env->fw);
if (res != fstrm_res_success)
return (ISC_R_FAILURE);
ioqversion++;
if (roll >= 0) {
if (env->mode == dns_dtmode_file && roll >= 0) {
/*
* Create a temporary isc_logfile_t structure so we can
* take advantage of the logfile rolling facility.
......@@ -299,13 +345,31 @@ dns_dt_reopen(dns_dtenv_t *env, int roll) {
file.maximum_reached = ISC_FALSE;
result = isc_logfile_roll(&file);
isc_mem_free(env->mctx, filename);
if (result != ISC_R_SUCCESS)
return (result);
CHECK(result);
}
fstrm_writer_open(env->fw);
env->iothr = fstrm_iothr_init(env->fopt, &fw);
if (env->iothr == NULL) {
isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSTAP,
DNS_LOGMODULE_DNSTAP, ISC_LOG_WARNING,
"unable to initialize dnstap I/O thread");
CHECK(ISC_R_FAILURE);
}
return (ISC_R_SUCCESS);
cleanup:
if (ffwopt != NULL)
fstrm_file_options_destroy(&ffwopt);
if (fw != NULL)
fstrm_writer_destroy(&fw);
if (fwopt != NULL)
fstrm_writer_options_destroy(&fwopt);
if (fuwopt != NULL)
fstrm_unix_writer_options_destroy(&fuwopt);
return (result);
}
static isc_result_t
......@@ -350,25 +414,46 @@ dns_dt_setversion(dns_dtenv_t *env, const char *version) {
static struct fstrm_iothr_queue *
dt_queue(dns_dtenv_t *env) {
isc_result_t result;
struct fstrm_iothr_queue *ioq;
struct ioq {
unsigned int ioqversion;
struct fstrm_iothr_queue *ioq;
} *ioq;
REQUIRE(VALID_DTENV(env));
if (env->iothr == NULL)
return (NULL);
result = dt_init();
if (result != ISC_R_SUCCESS)
return (NULL);
ioq = (struct fstrm_iothr_queue *) isc_thread_key_getspecific(dt_key);
ioq = (struct ioq *)isc_thread_key_getspecific(dt_key);
if (ioq != NULL && ioq->ioqversion != ioqversion) {
result = isc_thread_key_setspecific(dt_key, NULL);
if (result != ISC_R_SUCCESS)
return (NULL);
free(ioq);
ioq = NULL;
}
if (ioq == NULL) {
ioq = fstrm_iothr_get_input_queue(env->iothr);
if (ioq != NULL) {
result = isc_thread_key_setspecific(dt_key, ioq);
if (result != ISC_R_SUCCESS)
ioq = NULL;
ioq = malloc(sizeof(*ioq));
if (ioq == NULL)
return (NULL);
ioq->ioqversion = ioqversion;
ioq->ioq = fstrm_iothr_get_input_queue(env->iothr);
if (ioq->ioq == NULL) {
free(ioq);
return (NULL);
}
result = isc_thread_key_setspecific(dt_key, ioq);
if (result != ISC_R_SUCCESS) {
free(ioq);
return (NULL);
}
}
return (ioq);
return (ioq->ioq);
}
void
......@@ -399,7 +484,12 @@ destroy(dns_dtenv_t *env) {
"closing dnstap");
env->magic = 0;
fstrm_iothr_destroy(&env->iothr);
ioqversion++;
if (env->iothr != NULL)
fstrm_iothr_destroy(&env->iothr);
if (env->fopt != NULL)
fstrm_iothr_options_destroy(&env->fopt);
if (env->identity.base != NULL) {
isc_mem_free(env->mctx, env->identity.base);
......@@ -753,14 +843,22 @@ dnstap_file(struct fstrm_reader *r) {
}
isc_result_t
dns_dt_open(const char *filename, dns_dtmode_t mode, dns_dthandle_t *handle) {
dns_dt_open(const char *filename, dns_dtmode_t mode, isc_mem_t *mctx,
dns_dthandle_t **handlep)
{
isc_result_t result;
struct fstrm_file_options *fopt;
struct fstrm_file_options *fopt = NULL;
fstrm_res res;
dns_dthandle_t *handle;
REQUIRE(handle != NULL);
REQUIRE(handlep != NULL && *handlep == NULL);
handle = isc_mem_get(mctx, sizeof(*handle));
if (handle == NULL)
CHECK(ISC_R_NOMEMORY);
handle->mode = mode;
handle->mctx = NULL;
switch(mode) {
case dns_dtmode_file:
......@@ -787,7 +885,10 @@ dns_dt_open(const char *filename, dns_dtmode_t mode, dns_dthandle_t *handle) {
INSIST(0);
}
isc_mem_attach(mctx, &handle->mctx);
result = ISC_R_SUCCESS;
*handlep = handle;
handle = NULL;
cleanup:
if (result != ISC_R_SUCCESS && handle->reader != NULL) {
......@@ -796,6 +897,8 @@ dns_dt_open(const char *filename, dns_dtmode_t mode, dns_dthandle_t *handle) {
}
if (fopt != NULL)
fstrm_file_options_destroy(&fopt);
if (handle != NULL)
isc_mem_put(mctx, handle, sizeof(*handle));
return (result);
}
......@@ -825,13 +928,19 @@ dns_dt_getframe(dns_dthandle_t *handle, isc_uint8_t **bufp, size_t *sizep) {
}
void
dns_dt_close(dns_dthandle_t *handle) {
REQUIRE(handle != NULL);
dns_dt_close(dns_dthandle_t **handlep) {
dns_dthandle_t *handle;
REQUIRE(handlep != NULL && *handlep != NULL);
handle = *handlep;
*handlep = NULL;
if (handle->reader != NULL) {
fstrm_reader_destroy(&handle->reader);
handle->reader = NULL;
}
isc_mem_putanddetach(&handle->mctx, handle, sizeof(*handle));
}
isc_result_t
......
......@@ -82,10 +82,7 @@ typedef enum {
dns_dtmode_unix
} dns_dtmode_t;
typedef struct dns_dthandle {
dns_dtmode_t mode;
struct fstrm_reader *reader;
} dns_dthandle_t;
typedef struct dns_dthandle dns_dthandle_t;
#ifdef HAVE_DNSTAP
struct dns_dtdata {
......@@ -114,7 +111,7 @@ struct dns_dtdata {
isc_result_t
dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path,
struct fstrm_iothr_options *fopt, dns_dtenv_t **envp);
struct fstrm_iothr_options **foptp, dns_dtenv_t **envp);
/*%<
* Create and initialize the dnstap environment.
*
......@@ -128,10 +125,11 @@ dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path,
* with "file:", then dnstap logs are sent to a file instead of a
* socket.
*
*\li 'fopt' set the options for fstrm_iothr_init(). 'fopt' must have
*\li '*foptp' set the options for fstrm_iothr_init(). '*foptp' must have
* have had the number of input queues set and this should be set
* to the number of worker threads. Additionally the queue model
* should also be set. Other options may be set if desired.
* If dns_dt_create succeeds the *foptp is set to NULL.
*
* Requires:
*
......@@ -162,6 +160,8 @@ dns_dt_reopen(dns_dtenv_t *env, int roll);
* keep. If 'roll' is negative, or if 'env->mode' is dns_dtmode_unix,
* then the channel is simply reopened.
*
* Note: dns_dt_reopen() must be called in task exclusive mode.
*
* Requires:
*\li 'env' is a valid dnstap environment.
*/
......@@ -302,7 +302,8 @@ dns_dtdata_free(dns_dtdata_t **dp);
*/
isc_result_t
dns_dt_open(const char *filename, dns_dtmode_t mode, dns_dthandle_t *handle);
dns_dt_open(const char *filename, dns_dtmode_t mode,
isc_mem_t *mctx, dns_dthandle_t **handlep);
/*%<
* Opens a dnstap framestream at 'filename' and stores a pointer to the
* reader object in a dns_dthandle_t structure.
......@@ -314,7 +315,11 @@ dns_dt_open(const char *filename, dns_dtmode_t mode, dns_dthandle_t *handle);
*
* Requires:
*
*\li 'handle' is not NULL
*\li 'filename' is not NULL.
*
*\li 'handlep' is not NULL and '*handlep' is NULL.
*
*\li '*mctx' is not a valid memory context.
*
* Returns:
*
......@@ -350,13 +355,13 @@ dns_dt_getframe(dns_dthandle_t *handle, isc_uint8_t **bufp, size_t *sizep);
*/
void
dns_dt_close(dns_dthandle_t *handle);
dns_dt_close(dns_dthandle_t **handlep);
/*%<
* Closes the dnstap file referenced by 'handle'.
*
* Requires:
*
*\li 'handle' is not NULL
*\li '*handlep' is not NULL
*/
#endif /* _DNSTAP_H */
......@@ -69,28 +69,43 @@ ATF_TC_BODY(create, tc) {
ATF_REQUIRE(fopt != NULL);
fstrm_iothr_options_set_num_input_queues(fopt, 1);
result = dns_dt_create(mctx, dns_dtmode_file, TAPFILE, fopt, &dtenv);
result = dns_dt_create(mctx, dns_dtmode_file, TAPFILE, &fopt, &dtenv);
ATF_CHECK_EQ(result, ISC_R_SUCCESS);
if (dtenv != NULL)
dns_dt_detach(&dtenv);
if (fopt != NULL)
fstrm_iothr_options_destroy(&fopt);
ATF_CHECK(isc_file_exists(TAPFILE));
result = dns_dt_create(mctx, dns_dtmode_unix, TAPSOCK, fopt, &dtenv);
fopt = fstrm_iothr_options_init();
ATF_REQUIRE(fopt != NULL);
fstrm_iothr_options_set_num_input_queues(fopt, 1);
result = dns_dt_create(mctx, dns_dtmode_unix, TAPSOCK, &fopt, &dtenv);
ATF_CHECK_EQ(result, ISC_R_SUCCESS);
if (dtenv != NULL)
dns_dt_detach(&dtenv);
if (fopt != NULL)
fstrm_iothr_options_destroy(&fopt);
/* 'create' should succeed, but the file shouldn't exist yet */
ATF_CHECK(!isc_file_exists(TAPSOCK));
result = dns_dt_create(mctx, 33, TAPSOCK, fopt, &dtenv);
fopt = fstrm_iothr_options_init();
ATF_REQUIRE(fopt != NULL);
fstrm_iothr_options_set_num_input_queues(fopt, 1);
result = dns_dt_create(mctx, 33, TAPSOCK, &fopt, &dtenv);
ATF_CHECK_EQ(result, ISC_R_FAILURE);
ATF_CHECK_EQ(dtenv, NULL);
if (dtenv != NULL)
dns_dt_detach(&dtenv);
if (fopt != NULL)
fstrm_iothr_options_destroy(&fopt);
cleanup();
fstrm_iothr_options_destroy(&fopt);
dns_dt_shutdown();
dns_test_end();
}
......@@ -102,7 +117,7 @@ ATF_TC_HEAD(send, tc) {
ATF_TC_BODY(send, tc) {
isc_result_t result;
dns_dtenv_t *dtenv = NULL;
dns_dthandle_t handle;
dns_dthandle_t *handle = NULL;
isc_uint8_t *data;
size_t dsize;