Skip to content

Commit

Permalink
aio: fix data race in aio
Browse files Browse the repository at this point in the history
This particular problem reported by the sanitizer was unlikely to
have any real impact, but using this boolean and avoiding the
clearing of the expire_q avoids a possible NULL pointer dereference.
  • Loading branch information
gdamore committed Dec 2, 2024
1 parent dcffb02 commit f17009f
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 14 deletions.
24 changes: 14 additions & 10 deletions src/core/aio.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
//

#include "core/nng_impl.h"
#include "core/platform.h"
#include "nng/nng.h"
#include <string.h>

Expand Down Expand Up @@ -91,6 +92,12 @@ nni_aio_init(nni_aio *aio, nni_cb cb, void *arg)
aio->a_init = true;
}

static bool
aio_stopped(nni_aio *aio)
{
return (nni_atomic_get_bool(&aio->a_stopped));
}

void
nni_aio_fini(nni_aio *aio)
{
Expand Down Expand Up @@ -125,7 +132,7 @@ nni_aio_free(nni_aio *aio)
void
nni_aio_reap(nni_aio *aio)
{
if (aio != NULL && aio->a_expire_q != NULL) {
if (aio != NULL && aio->a_init) {
nni_reap(&aio_reap_list, aio);
}
}
Expand Down Expand Up @@ -158,7 +165,7 @@ nni_aio_set_iov(nni_aio *aio, unsigned nio, const nni_iov *iov)
void
nni_aio_stop(nni_aio *aio)
{
if (aio != NULL && aio->a_expire_q != NULL) {
if (aio != NULL && aio->a_init && !aio_stopped(aio)) {
nni_aio_cancel_fn fn;
void *arg;
nni_aio_expire_q *eq = aio->a_expire_q;
Expand All @@ -180,17 +187,14 @@ nni_aio_stop(nni_aio *aio)

nni_aio_wait(aio);

// Disconnect us from the expire queue.
// We cannot be used any more. This reduces
// lock contention during teardown.
aio->a_expire_q = NULL;
nni_atomic_set_bool(&aio->a_stopped, true);
}
}

void
nni_aio_close(nni_aio *aio)
{
if (aio != NULL && aio->a_expire_q != NULL) {
if (aio != NULL && aio->a_init && !aio_stopped(aio)) {
nni_aio_cancel_fn fn;
void *arg;
nni_aio_expire_q *eq = aio->a_expire_q;
Expand Down Expand Up @@ -293,7 +297,7 @@ nni_aio_count(nni_aio *aio)
void
nni_aio_wait(nni_aio *aio)
{
if (aio != NULL && aio->a_expire_q != NULL) {
if (aio != NULL && aio->a_init && !aio_stopped(aio)) {
nni_task_wait(&aio->a_task);
}
}
Expand All @@ -314,7 +318,7 @@ nni_aio_begin(nni_aio *aio)
// checks may wish ignore or suppress these checks.
nni_aio_expire_q *eq = aio->a_expire_q;

if (eq == NULL) {
if (aio_stopped(aio)) {
NNI_ASSERT(!nni_list_node_active(&aio->a_expire_node));
aio->a_result = NNG_ECANCELED;
aio->a_cancel_fn = NULL;
Expand All @@ -341,7 +345,7 @@ nni_aio_begin(nni_aio *aio)
aio->a_expire = NNI_TIME_NEVER;
aio->a_sleep = false;
aio->a_expire_ok = false;
aio->a_expire_q = NULL;
nni_atomic_set_bool(&aio->a_stopped, true);
nni_mtx_unlock(&eq->eq_mtx);

return (NNG_ECANCELED);
Expand Down
12 changes: 8 additions & 4 deletions src/core/aio.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include "core/defs.h"
#include "core/list.h"
#include "core/platform.h"
#include "core/reap.h"
#include "core/taskq.h"
#include "core/thread.h"
Expand Down Expand Up @@ -219,7 +220,8 @@ struct nng_aio {
bool a_expiring; // Expiration in progress
bool a_use_expire; // Use expire instead of timeout
bool a_init; // This is initialized
nni_task a_task;

nni_task a_task;

// Read/write operations.
nni_iov a_iov[8];
Expand All @@ -234,14 +236,16 @@ struct nng_aio {
void *a_inputs[4];
void *a_outputs[4];

nni_atomic_bool a_stopped;
nni_aio_expire_q *a_expire_q;
nni_list_node a_expire_node; // Expiration node
nni_reap_node a_reap_node;

// Provider-use fields.
nni_aio_cancel_fn a_cancel_fn;
void *a_cancel_arg;
void *a_prov_data;
nni_list_node a_prov_node; // Linkage on provider list.
nni_aio_expire_q *a_expire_q;
nni_list_node a_expire_node; // Expiration node
nni_reap_node a_reap_node;
};

#endif // CORE_AIO_H

0 comments on commit f17009f

Please sign in to comment.