From a373f854554186ac5e93cbd27fbb40c89a0fc4bb Mon Sep 17 00:00:00 2001 From: willwu1217 <148855842+willwu1217@users.noreply.github.com> Date: Tue, 24 Oct 2023 17:00:58 +0800 Subject: [PATCH 1/2] Unlocking of sub0_sock lock after aio is deleted from the finish list Assume that sub0_ctx_cancel and sub0_recv_cb are executed concurrently. When sub0_recv_cb is unlocked, and before aio is deleted from the finish queue. nni_list_active in sub0_ctx_cancel may mistakenly believe that aio is still on recv_queue, thus triggering repeated execution of nni_list_remove. The second execution will trigger a null pointer access. So unlocking after aio is deleted from the finish list. --- src/sp/protocol/pubsub0/sub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sp/protocol/pubsub0/sub.c b/src/sp/protocol/pubsub0/sub.c index 10f42724d..42e0ead8f 100644 --- a/src/sp/protocol/pubsub0/sub.c +++ b/src/sp/protocol/pubsub0/sub.c @@ -388,7 +388,6 @@ sub0_recv_cb(void *arg) nni_pollable_raise(&sock->readable); } } - nni_mtx_unlock(&sock->lk); // NB: This is slightly less efficient in that we may have // created an extra copy in the face of e.g. two subscriptions, @@ -405,6 +404,7 @@ sub0_recv_cb(void *arg) nni_list_remove(&finish, aio); nni_aio_finish_sync(aio, 0, len); } + nni_mtx_unlock(&sock->lk); nni_pipe_recv(p->pipe, &p->aio_recv); } From 436e217c09ae4b87b304608ed2126008f87a0e22 Mon Sep 17 00:00:00 2001 From: willwu1217 Date: Wed, 25 Oct 2023 14:23:16 +0800 Subject: [PATCH 2/2] Add finish nni_list in struct nng_aio 1. Add finish nni_list in struct nng_aio. 2. Use a_finish_node to store finished aio in function sub0_recv_cb, thus separated from sub0_ctx recv_queue. --- src/core/aio.h | 1 + src/sp/protocol/pubsub0/sub.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/aio.h b/src/core/aio.h index 6315e90c9..667098a6a 100644 --- a/src/core/aio.h +++ b/src/core/aio.h @@ -209,6 +209,7 @@ struct nng_aio { nni_aio_expire_q *a_expire_q; nni_list_node a_expire_node; // Expiration node nni_reap_node a_reap_node; + nni_list_node a_finish_node; // Finished node }; #endif // CORE_AIO_H diff --git a/src/sp/protocol/pubsub0/sub.c b/src/sp/protocol/pubsub0/sub.c index 42e0ead8f..7c1e408e6 100644 --- a/src/sp/protocol/pubsub0/sub.c +++ b/src/sp/protocol/pubsub0/sub.c @@ -326,7 +326,7 @@ sub0_recv_cb(void *arg) return; } - nni_aio_list_init(&finish); + NNI_LIST_INIT(&finish, nng_aio, a_finish_node); msg = nni_aio_get_msg(&p->aio_recv); nni_aio_set_msg(&p->aio_recv, NULL); @@ -388,6 +388,7 @@ sub0_recv_cb(void *arg) nni_pollable_raise(&sock->readable); } } + nni_mtx_unlock(&sock->lk); // NB: This is slightly less efficient in that we may have // created an extra copy in the face of e.g. two subscriptions, @@ -404,7 +405,6 @@ sub0_recv_cb(void *arg) nni_list_remove(&finish, aio); nni_aio_finish_sync(aio, 0, len); } - nni_mtx_unlock(&sock->lk); nni_pipe_recv(p->pipe, &p->aio_recv); }