From 68d1a15593ef94b38dea365f7b471ab2b675f83a Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Wed, 2 Sep 2026 16:52:29 +0200 Subject: [PATCH 1/7] compat FEATURE add acquire/release atomic macros ATOMIC_STORE_RELEASE() and ATOMIC_LOAD_ACQUIRE() are needed to hand over ownership of a shared object between two threads without a mutex. The __sync fallback has no load/store builtins, so a release store is an explicit barrier followed by a plain store and an acquire load is a no-op fetch-and-add, which is already a full barrier. --- compat/compat.h.in | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/compat/compat.h.in b/compat/compat.h.in index cbea52af..4059f918 100644 --- a/compat/compat.h.in +++ b/compat/compat.h.in @@ -124,6 +124,8 @@ # define ATOMIC_DEC_RELAXED(var) atomic_fetch_sub_explicit(&(var), 1, memory_order_relaxed) # define ATOMIC_DEC_ACQ_REL(var) atomic_fetch_sub_explicit(&(var), 1, memory_order_acq_rel) # define ATOMIC_SUB_RELAXED(var, x) atomic_fetch_sub_explicit(&(var), x, memory_order_relaxed) +# define ATOMIC_STORE_RELEASE(var, x) atomic_store_explicit(&(var), x, memory_order_release) +# define ATOMIC_LOAD_ACQUIRE(var) atomic_load_explicit(&(var), memory_order_acquire) # define ATOMIC_PTR_COMPARE_EXCHANGE_RELAXED(var, exp, des, result) \ result = atomic_compare_exchange_strong_explicit((ATOMIC_PTR_T *)&(var), &(exp), des, memory_order_relaxed, \ @@ -148,6 +150,10 @@ /* __sync_fetch_and_sub() is already a full barrier */ # define ATOMIC_DEC_ACQ_REL(var) __sync_fetch_and_sub(&(var), 1) # define ATOMIC_SUB_RELAXED(var, x) __sync_fetch_and_sub(&(var), x) +/* there are no __sync load/store builtins, so a release store is an explicit barrier followed by a plain + * store and an acquire load is a no-op RMW, which is already a full barrier */ +# define ATOMIC_STORE_RELEASE(var, x) (__sync_synchronize(), (var) = (x)) +# define ATOMIC_LOAD_ACQUIRE(var) __sync_fetch_and_add(&(var), 0) # define ATOMIC_PTR_COMPARE_EXCHANGE_RELAXED(var, exp, des, result) \ { \ From 5caf2516f88c61fe890158a6aede0f98de44e6fa Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Wed, 2 Sep 2026 16:52:29 +0200 Subject: [PATCH 2/7] session BUGFIX access session status atomically ThreadSanitizer reports a data race on nc_session.status between the Call Home thread reading it in nc_server_ch_client_thread_session_cond_wait() under ch_lock and a poll thread writing it in nc_server_send_reply_io() under rpc_lock. There is no lock held by every thread that uses a session, and there cannot be one without introducing a lock ordering problem with rpc_lock/io_lock, so status is simply accessed lock-free by design. Make that explicit and free of undefined behaviour by turning status into an ATOMIC_T accessed through NC_SESSION_STATUS_GET/SET(). Silencing the race requires converting every access, not just the two the sanitizer happened to pair up. term_reason is written next to status on almost every path and read by the same poll thread, so it has exactly the same problem and is converted along with it. Refs #635 --- src/io.c | 62 +++++++++--------- src/proxy_unix.c | 4 +- src/session.c | 16 ++--- src/session_client.c | 30 ++++----- src/session_client_ssh.c | 12 ++-- src/session_client_tls.c | 8 +-- src/session_mbedtls.c | 8 +-- src/session_openssl.c | 16 ++--- src/session_p.h | 48 +++++++++++++- src/session_server.c | 90 +++++++++++++------------- src/session_server_ssh.c | 16 ++--- src/session_server_ssh_auth_callback.c | 6 +- src/session_server_ssh_auth_message.c | 10 +-- tests/test_fd_comm.c | 4 +- tests/test_io.c | 2 +- 15 files changed, 188 insertions(+), 144 deletions(-) diff --git a/src/io.c b/src/io.c index 7c499506..9c5e0b55 100644 --- a/src/io.c +++ b/src/io.c @@ -80,7 +80,7 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti assert(session); assert(buf); - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { return -1; } @@ -110,14 +110,14 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti break; } else { ERR(session, "Reading from file descriptor (%d) failed (%s).", fd, strerror(errno)); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); return -1; } } else if (r == 0) { ERR(session, "Communication file descriptor (%d) unexpectedly closed.", fd); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); return -1; } break; @@ -131,14 +131,14 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti break; } else if (r == SSH_ERROR) { ERR(session, "Reading from the SSH channel failed (%s).", ssh_get_error(session->ti.libssh.session)); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); return -1; } else if (r == 0) { if (ssh_channel_is_eof(session->ti.libssh.channel)) { ERR(session, "SSH channel unexpected EOF."); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); return -1; } break; @@ -166,8 +166,8 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti } else { ERR(session, "Active read timeout elapsed."); } - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); return -1; } } else { @@ -271,7 +271,7 @@ nc_read_msg_io(struct nc_session *session, int io_timeout, int passing_io_lock, /* use timeout in milliseconds instead seconds */ inact_timeout = NC_READ_INACT_TIMEOUT * 1000; - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { ERR(session, "Invalid session to read from."); ret = -1; goto cleanup; @@ -386,7 +386,7 @@ nc_read_poll(struct nc_session *session, int io_timeout) int ret = -2; struct pollfd fds; - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { ERR(session, "Invalid session to poll."); return -1; } @@ -403,13 +403,13 @@ nc_read_poll(struct nc_session *session, int io_timeout) ret = ssh_channel_poll_timeout(session->ti.libssh.channel, io_timeout, 0); if (ret == SSH_ERROR) { ERR(session, "SSH channel poll error (%s).", ssh_get_error(session->ti.libssh.session)); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); return -1; } else if (ret == SSH_EOF) { ERR(session, "SSH channel unexpected EOF."); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); return -1; } else if (ret > 0) { /* fake it */ @@ -454,23 +454,23 @@ nc_read_poll(struct nc_session *session, int io_timeout) if (ret < 0) { /* poll failed - something really bad happened, close the session */ ERR(session, "poll error (%s).", strerror(errno)); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); return -1; } else { /* in case of standard (non-libssh) poll, there still can be an error */ if (fds.revents & POLLERR) { ERR(session, "Communication channel error."); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); return -1; } /* Some poll() implementations may return POLLHUP|POLLIN when the other * side has closed but there is data left to read in the buffer. */ if ((fds.revents & POLLHUP) && !(fds.revents & POLLIN)) { ERR(session, "Communication channel unexpectedly closed."); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); return -1; } } @@ -488,7 +488,7 @@ nc_read_msg_poll_io(struct nc_session *session, int io_timeout, struct ly_in **m assert(msg); *msg = NULL; - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { ERR(session, "Invalid session to read from."); return -1; } @@ -588,15 +588,15 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) int c, fd, interrupted; uint32_t written = 0; - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { return -1; } /* prevent SIGPIPE this way */ if (!nc_session_is_connected(session)) { ERR(session, "Communication socket unexpectedly closed."); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); return -1; } @@ -624,8 +624,8 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) case NC_TI_SSH: if (ssh_channel_is_closed(session->ti.libssh.channel)) { ERR(session, "SSH channel unexpectedly closed."); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); return -1; } c = ssh_channel_write(session->ti.libssh.channel, (char *)buf + written, count - written); @@ -854,7 +854,7 @@ nc_write_msg_io(struct nc_session *session, int io_timeout, int type, ...) assert(session); - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { ERR(session, "Invalid session to write to."); return NC_MSG_ERROR; } @@ -1078,7 +1078,7 @@ nc_write_msg_io(struct nc_session *session, int io_timeout, int type, ...) /* flush message */ nc_write_clb((void *)&arg, NULL, 0, 0); - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { /* error was already written */ ret = NC_MSG_ERROR; } else { diff --git a/src/proxy_unix.c b/src/proxy_unix.c index c6c67ae6..38ecd46f 100644 --- a/src/proxy_unix.c +++ b/src/proxy_unix.c @@ -122,7 +122,7 @@ nc_proxy_read_msg(int fd, NC_PROT_VERSION version, int timeout_ms, char **buf, u } /* fill dummy session (id 0 causes session not to be included in log messages) */ - sess.status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(&sess, NC_STATUS_RUNNING); sess.version = version; sess.ti_type = NC_TI_UNIX; sess.ti.unixsock.sock = fd; @@ -154,7 +154,7 @@ nc_proxy_write_msg(int fd, NC_PROT_VERSION version, const char *buf, uint32_t bu struct nc_wclb_arg warg = {.session = &sess}; /* fill dummy session (id 0 causes session not to be included in log messages) */ - sess.status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(&sess, NC_STATUS_RUNNING); sess.version = version; sess.ti_type = NC_TI_UNIX; sess.ti.unixsock.sock = fd; diff --git a/src/session.c b/src/session.c index d5954fc9..e05d823d 100644 --- a/src/session.c +++ b/src/session.c @@ -562,7 +562,7 @@ nc_session_get_status(const struct nc_session *session) { NC_CHECK_ARG_RET(session, session, NC_STATUS_ERR); - return session->status; + return NC_SESSION_STATUS_GET(session); } API NC_SESSION_TERM_REASON @@ -570,7 +570,7 @@ nc_session_get_term_reason(const struct nc_session *session) { NC_CHECK_ARG_RET(session, session, NC_SESSION_TERM_ERR); - return session->term_reason; + return NC_SESSION_TERM_REASON_GET(session); } API uint32_t @@ -943,7 +943,7 @@ nc_session_free_transport(struct nc_session *session, int *multisession) if (session->ti.libssh.channel) { if ((session->side == NC_CLIENT) || - ((session->side == NC_SERVER) && (session->term_reason == NC_SESSION_TERM_CLOSED))) { + ((session->side == NC_SERVER) && (NC_SESSION_TERM_REASON_GET(session) == NC_SESSION_TERM_CLOSED))) { /* NC_SERVER: session was properly closed by the client, so he should have sent SSH channel EOF. * Polling here should properly set libssh internal state and avoid libssh WRN log about writing * to a closed channel in ssh_channel_free(). @@ -959,7 +959,7 @@ nc_session_free_transport(struct nc_session *session, int *multisession) if (session->ti.libssh.next) { for (siter = session->ti.libssh.next; siter != session; siter = siter->ti.libssh.next) { - if (siter->status != NC_STATUS_STARTING) { + if (NC_SESSION_STATUS_GET(siter) != NC_STATUS_STARTING) { *multisession = 1; break; } @@ -1100,7 +1100,7 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) } /* store status, so we can check if this session is already closing */ - status = session->status; + status = NC_SESSION_STATUS_GET(session); if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { /* CH UNLOCK */ @@ -1147,9 +1147,9 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) /* notify the peer that we're closing the session, either if: * - session running - normal disconnect from client * - session invalid - client disconnected from a Call Home session */ - if ((session->status == NC_STATUS_RUNNING) || + if ((NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING) || ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME) && - (session->status == NC_STATUS_INVALID) && (session->term_reason == NC_SESSION_TERM_CLOSED))) { + (NC_SESSION_STATUS_GET(session) == NC_STATUS_INVALID) && (NC_SESSION_TERM_REASON_GET(session) == NC_SESSION_TERM_CLOSED))) { if (session->side == NC_CLIENT) { /* graceful close: + transport shutdown indication */ nc_session_free_client_close_graceful(session); @@ -1168,7 +1168,7 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) } /* mark session for closing */ - session->status = NC_STATUS_CLOSING; + NC_SESSION_STATUS_SET(session, NC_STATUS_CLOSING); if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CH_THREAD)) { /* signaling a condition does not require its mutex to be held */ diff --git a/src/session_client.c b/src/session_client.c index a947d569..da789987 100644 --- a/src/session_client.c +++ b/src/session_client.c @@ -850,7 +850,7 @@ get_oper_data(struct nc_session *session, int has_get_data, const char *filter, lyd_free_tree(envp); lyd_free_tree(op); - if (session->status != NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) { /* something bad happened, discard the session */ ERR(session, "Invalid session, discarding."); ret = -1; @@ -1090,7 +1090,7 @@ nc_ctx_fill(struct nc_session *session, struct module_info *modules, ly_module_i user_clb, user_data, has_get_schema, &mod); if (!mod) { - if (session->status != NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) { /* something bad heppened, discard the session */ ERR(session, "Invalid session, discarding."); goto cleanup; @@ -1323,7 +1323,7 @@ nc_connect_inout(int fdin, int fdout, struct ly_ctx *ctx) /* prepare session structure */ session = nc_new_session(NC_CLIENT, 0); NC_CHECK_ERRMEM_RET(!session, NULL); - session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(session, NC_STATUS_STARTING); /* transport specific data */ session->ti_type = NC_TI_FD; @@ -1339,7 +1339,7 @@ nc_connect_inout(int fdin, int fdout, struct ly_ctx *ctx) if (nc_handshake_io(session) != NC_MSG_HELLO) { goto fail; } - session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(session, NC_STATUS_RUNNING); if (nc_ctx_check_and_fill(session) == -1) { goto fail; @@ -1435,7 +1435,7 @@ nc_connect_unix(const char *address, struct ly_ctx *ctx) /* prepare session structure */ session = nc_new_session(NC_CLIENT, 0); NC_CHECK_ERRMEM_GOTO(!session, , fail); - session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(session, NC_STATUS_STARTING); /* transport specific data */ session->ti_type = NC_TI_UNIX; @@ -1478,7 +1478,7 @@ nc_connect_unix(const char *address, struct ly_ctx *ctx) if (nc_handshake_io(session) != NC_MSG_HELLO) { goto fail; } - session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(session, NC_STATUS_RUNNING); if (nc_ctx_check_and_fill(session) == -1) { goto fail; @@ -2372,7 +2372,7 @@ nc_recv_reply(struct nc_session *session, struct nc_rpc *rpc, uint64_t msgid, in NC_CHECK_ARG_RET(session, session, rpc, envp, op, NC_MSG_ERROR); - if ((session->status != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { ERR(session, "Invalid session to receive RPC replies."); return NC_MSG_ERROR; } @@ -2431,7 +2431,7 @@ nc_recv_notif(struct nc_session *session, int timeout, struct lyd_node **envp, s { NC_CHECK_ARG_RET(session, session, envp, op, NC_MSG_ERROR); - if ((session->status != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { ERR(session, "Invalid session to receive Notifications."); return NC_MSG_ERROR; } @@ -2473,7 +2473,7 @@ nc_recv_notif_thread(void *arg) } lyd_free_all(envp); lyd_free_all(op); - } else if ((msgtype == NC_MSG_ERROR) && (session->status != NC_STATUS_RUNNING)) { + } else if ((msgtype == NC_MSG_ERROR) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING)) { /* quit this thread once the session is broken */ break; } @@ -2506,7 +2506,7 @@ nc_recv_notif_dispatch_data(struct nc_session *session, nc_notif_dispatch_clb no NC_CHECK_ARG_RET(session, session, notif_clb, -1); - if ((session->status != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { ERR(session, "Invalid session to receive Notifications."); return -1; } @@ -2545,7 +2545,7 @@ nc_recv_msg(struct nc_session *session, int timeout, char **msg) NC_CHECK_ARG_RET(session, session, msg, NC_MSG_ERROR); - if ((session->status != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { ERR(session, "Invalid session to receive RPC replies."); return NC_MSG_ERROR; } @@ -2644,7 +2644,7 @@ nc_send_rpc(struct nc_session *session, struct nc_rpc *rpc, int timeout, uint64_ NC_CHECK_ARG_RET(session, session, rpc, msgid, NC_MSG_ERROR); - if ((session->status != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { ERR(session, "Invalid session to send RPCs."); return NC_MSG_ERROR; } @@ -3227,7 +3227,7 @@ nc_send_msg(struct nc_session *session, const char *msg, uint32_t msg_len, int t NC_CHECK_ARG_RET(session, session, msg, NC_MSG_ERROR); - if ((session->status != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) || (session->side != NC_CLIENT)) { ERR(session, "Invalid session to send RPCs."); return NC_MSG_ERROR; } @@ -3442,8 +3442,8 @@ nc_client_monitoring_thread(void *arg) if (mtarg->pfds[i].revents & (POLLHUP | POLLNVAL)) { /* save the session and stop monitoring it, callback will be called outside of the lock */ session = mtarg->sessions[i]; - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); nc_client_monitoring_session_stop(session, 0); break; } diff --git a/src/session_client_ssh.c b/src/session_client_ssh.c index ff7cb027..c5d45c4a 100644 --- a/src/session_client_ssh.c +++ b/src/session_client_ssh.c @@ -1684,7 +1684,7 @@ _nc_connect_libssh(ssh_session ssh_session, struct ly_ctx *ctx, struct nc_keepal /* prepare session structure */ session = nc_new_session(NC_CLIENT, 0); NC_CHECK_ERRMEM_RET(!session, NULL); - session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(session, NC_STATUS_STARTING); session->ti_type = NC_TI_SSH; session->ti.libssh.session = ssh_session; @@ -1774,7 +1774,7 @@ _nc_connect_libssh(ssh_session ssh_session, struct ly_ctx *ctx, struct nc_keepal if (nc_handshake_io(session) != NC_MSG_HELLO) { goto fail; } - session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(session, NC_STATUS_RUNNING); if (nc_ctx_check_and_fill(session) == -1) { goto fail; @@ -1833,7 +1833,7 @@ nc_connect_ssh(const char *host, uint16_t port, struct ly_ctx *ctx) /* prepare session structure */ session = nc_new_session(NC_CLIENT, 0); NC_CHECK_ERRMEM_GOTO(!session, , fail); - session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(session, NC_STATUS_STARTING); /* transport-specific data */ session->ti_type = NC_TI_SSH; @@ -1883,7 +1883,7 @@ nc_connect_ssh(const char *host, uint16_t port, struct ly_ctx *ctx) if (nc_handshake_io(session) != NC_MSG_HELLO) { goto fail; } - session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(session, NC_STATUS_RUNNING); if (nc_ctx_check_and_fill(session) == -1) { goto fail; @@ -1925,7 +1925,7 @@ nc_connect_ssh_channel(struct nc_session *session, struct ly_ctx *ctx) /* prepare session structure */ new_session = nc_new_session(NC_CLIENT, 1); NC_CHECK_ERRMEM_RET(!new_session, NULL); - new_session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(new_session, NC_STATUS_STARTING); /* share some parameters including the IO lock (we are using one socket for both sessions) */ new_session->ti_type = NC_TI_SSH; @@ -1960,7 +1960,7 @@ nc_connect_ssh_channel(struct nc_session *session, struct ly_ctx *ctx) if (nc_handshake_io(new_session) != NC_MSG_HELLO) { goto fail; } - new_session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(new_session, NC_STATUS_RUNNING); if (nc_ctx_check_and_fill(new_session) == -1) { goto fail; diff --git a/src/session_client_tls.c b/src/session_client_tls.c index 9dd441b9..0c0b5468 100644 --- a/src/session_client_tls.c +++ b/src/session_client_tls.c @@ -353,7 +353,7 @@ nc_connect_tls(const char *host, unsigned short port, struct ly_ctx *ctx) /* prepare session structure */ session = nc_new_session(NC_CLIENT, 0); NC_CHECK_ERRMEM_RET(!session, NULL); - session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(session, NC_STATUS_STARTING); /* create and assign socket */ sock = nc_sock_connect(NULL, 0, host, port, -1, &client_opts.ka, NULL, &ip_host); @@ -381,7 +381,7 @@ nc_connect_tls(const char *host, unsigned short port, struct ly_ctx *ctx) if (nc_handshake_io(session) != NC_MSG_HELLO) { goto fail; } - session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(session, NC_STATUS_RUNNING); if (nc_ctx_check_and_fill(session) == -1) { goto fail; @@ -416,7 +416,7 @@ nc_accept_callhome_tls_sock(int sock, const char *host, uint16_t port, struct ly /* prepare session structure */ session = nc_new_session(NC_CLIENT, 0); NC_CHECK_ERRMEM_RET(!session, NULL); - session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(session, NC_STATUS_STARTING); /* fill the session */ session->ti_type = NC_TI_TLS; @@ -437,7 +437,7 @@ nc_accept_callhome_tls_sock(int sock, const char *host, uint16_t port, struct ly if (nc_handshake_io(session) != NC_MSG_HELLO) { goto fail; } - session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(session, NC_STATUS_RUNNING); if (nc_ctx_check_and_fill(session) == -1) { goto fail; diff --git a/src/session_mbedtls.c b/src/session_mbedtls.c index e03d2065..ddde452a 100644 --- a/src/session_mbedtls.c +++ b/src/session_mbedtls.c @@ -1543,14 +1543,14 @@ nc_tls_read_wrap(struct nc_session *session, unsigned char *buf, size_t size) break; case MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY: nc_mbedtls_strerr(session, rc, "Communication socket unexpectedly closed"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); rc = -1; break; default: nc_mbedtls_strerr(session, rc, "TLS communication error occurred"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); rc = -1; break; } diff --git a/src/session_openssl.c b/src/session_openssl.c index e1808914..9afaf0af 100644 --- a/src/session_openssl.c +++ b/src/session_openssl.c @@ -1225,28 +1225,28 @@ nc_tls_read_wrap(struct nc_session *session, unsigned char *buf, size_t size) break; case SSL_ERROR_ZERO_RETURN: ERR(session, "Communication socket unexpectedly closed (OpenSSL)."); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); rc = -1; break; case SSL_ERROR_SYSCALL: ERR(session, "TLS socket error (%s).", errno ? strerror(errno) : "unexpected EOF"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); rc = -1; break; case SSL_ERROR_SSL: reasons = nc_tls_get_err_reasons(); ERR(session, "TLS communication error (%s).", reasons); free(reasons); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); rc = -1; break; default: ERR(session, "Unknown TLS error occurred (err code %d).", err); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); rc = -1; break; } diff --git a/src/session_p.h b/src/session_p.h index 6e0de203..611ec405 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -1030,8 +1030,12 @@ struct nc_msg_cont { * @brief NETCONF session structure */ struct nc_session { - NC_STATUS status; /**< status of the session */ - NC_SESSION_TERM_REASON term_reason; /**< reason of termination, if status is NC_STATUS_INVALID */ + ATOMIC_T status; /**< status of the session (::NC_STATUS), read/written by any thread using + the session without any lock, so always access it with ::NC_SESSION_STATUS_GET() + and ::NC_SESSION_STATUS_SET() */ + ATOMIC_T term_reason; /**< reason of termination (::NC_SESSION_TERM_REASON), if status is + NC_STATUS_INVALID, accessed lock-free just like @p status, so always use + ::NC_SESSION_TERM_REASON_GET() and ::NC_SESSION_TERM_REASON_SET() */ uint32_t killed_by; /**< session responsible for termination, if term_reason is NC_SESSION_TERM_KILLED */ NC_SIDE side; /**< side of the session: client or server */ @@ -1152,6 +1156,46 @@ struct nc_session { } opts; }; +/** + * @brief Get the status of a session. + * + * ::nc_session.status is read and written by every thread that uses the session (a poll thread, the user + * thread freeing it, a Call Home thread) and there is no single lock held by all of them, so it must + * always be accessed atomically. + * + * @param[in] session Session to read the status of. + * @return Current session status. + */ +#define NC_SESSION_STATUS_GET(session) \ + ((NC_STATUS)ATOMIC_LOAD_RELAXED(((struct nc_session *)(session))->status)) + +/** + * @brief Set the status of a session. + * + * @param[in] session Session to set the status of. + * @param[in] st Status (::NC_STATUS) to set. + */ +#define NC_SESSION_STATUS_SET(session, st) ATOMIC_STORE_RELAXED((session)->status, (uint32_t)(st)) + +/** + * @brief Get the termination reason of a session. + * + * Accessed by the same set of threads as ::nc_session.status, see ::NC_SESSION_STATUS_GET(). + * + * @param[in] session Session to read the termination reason of. + * @return Current session termination reason. + */ +#define NC_SESSION_TERM_REASON_GET(session) \ + ((NC_SESSION_TERM_REASON)ATOMIC_LOAD_RELAXED(((struct nc_session *)(session))->term_reason)) + +/** + * @brief Set the termination reason of a session. + * + * @param[in] session Session to set the termination reason of. + * @param[in] reason Termination reason (::NC_SESSION_TERM_REASON) to set. + */ +#define NC_SESSION_TERM_REASON_SET(session, reason) ATOMIC_STORE_RELAXED((session)->term_reason, (uint32_t)(reason)) + enum nc_ps_session_state { NC_PS_STATE_NONE = 0, /**< session is not being worked with */ NC_PS_STATE_BUSY, /**< session is being polled or communicated on (and locked) */ diff --git a/src/session_server.c b/src/session_server.c index 568691e2..18a3309d 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -282,16 +282,16 @@ nc_session_set_term_reason(struct nc_session *session, NC_SESSION_TERM_REASON re return; } - if ((reason != NC_SESSION_TERM_KILLED) && (session->term_reason == NC_SESSION_TERM_KILLED)) { + if ((reason != NC_SESSION_TERM_KILLED) && (NC_SESSION_TERM_REASON_GET(session) == NC_SESSION_TERM_KILLED)) { session->killed_by = 0; } - session->term_reason = reason; + NC_SESSION_TERM_REASON_SET(session, reason); } API void nc_session_set_killed_by(struct nc_session *session, uint32_t sid) { - if (!session || (session->term_reason != NC_SESSION_TERM_KILLED)) { + if (!session || (NC_SESSION_TERM_REASON_GET(session) != NC_SESSION_TERM_KILLED)) { ERRARG(session, "session"); return; } else if (!sid) { @@ -313,7 +313,7 @@ nc_session_set_status(struct nc_session *session, NC_STATUS status) return; } - session->status = status; + NC_SESSION_STATUS_SET(session, status); } API int @@ -1413,7 +1413,7 @@ nc_clb_default_get_schema(struct lyd_node *rpc, struct nc_session *session) API struct nc_server_reply * nc_clb_default_close_session(struct lyd_node *UNUSED(rpc), struct nc_session *session) { - session->term_reason = NC_SESSION_TERM_CLOSED; + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_CLOSED); return nc_server_reply_ok(); } @@ -1830,7 +1830,7 @@ nc_accept_inout(int fdin, int fdout, const char *username, const struct ly_ctx * /* prepare session structure */ *session = nc_new_session(NC_SERVER, 0); NC_CHECK_ERRMEM_RET(!(*session), NC_MSG_ERROR); - (*session)->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(*session, NC_STATUS_STARTING); /* transport specific data */ (*session)->ti_type = NC_TI_FD; @@ -1857,7 +1857,7 @@ nc_accept_inout(int fdin, int fdout, const char *username, const struct ly_ctx * nc_realtime_get(&ts_cur); (*session)->opts.server.session_start = ts_cur; - (*session)->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(*session, NC_STATUS_RUNNING); return msgtype; } @@ -2424,7 +2424,7 @@ nc_server_recv_rpc_io(struct nc_session *session, int io_timeout, struct nc_serv NC_CHECK_ARG_RET(session, session, rpc, NC_PSPOLL_ERROR); - if ((session->status != NC_STATUS_RUNNING) || (session->side != NC_SERVER)) { + if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) || (session->side != NC_SERVER)) { ERR(session, "Invalid session to receive RPCs."); return NC_PSPOLL_ERROR; } @@ -2478,9 +2478,9 @@ nc_server_recv_rpc_io(struct nc_session *session, int io_timeout, struct nc_serv nc_server_reply_free(reply); if (r != NC_MSG_REPLY) { ERR(session, "Failed to write reply (%s), terminating session.", nc_msgtype2str[r]); - if (session->status != NC_STATUS_INVALID) { - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + if (NC_SESSION_STATUS_GET(session) != NC_STATUS_INVALID) { + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); } } @@ -2598,8 +2598,8 @@ nc_server_send_reply_io(struct nc_session *session, int io_timeout, const struct } /* special case if term_reason was set in callback, last reply was sent (needed for if nothing else) */ - if ((session->status == NC_STATUS_RUNNING) && (session->term_reason != NC_SESSION_TERM_NONE)) { - session->status = NC_STATUS_INVALID; + if ((NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING) && (NC_SESSION_TERM_REASON_GET(session) != NC_SESSION_TERM_NONE)) { + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); } return ret; @@ -2622,7 +2622,7 @@ nc_ps_ssh_find_new_channel(struct nc_session *session) } for (new = session->ti.libssh.next; new != session; new = new->ti.libssh.next) { - if ((new->status == NC_STATUS_STARTING) && new->ti.libssh.channel && + if ((NC_SESSION_STATUS_GET(new) == NC_STATUS_STARTING) && new->ti.libssh.channel && (new->flags & NC_SESSION_SSH_SUBSYS_NETCONF)) { return 1; } @@ -2666,8 +2666,8 @@ nc_ps_poll_session_io(struct nc_session *session, int io_timeout, time_t now_mon if (!(session->flags & NC_SESSION_CALLHOME) && !nc_session_get_notif_status(session) && idle_timeout && (now_mono >= session->opts.server.last_rpc + idle_timeout)) { sprintf(msg, "Session idle timeout elapsed"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_TIMEOUT; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_TIMEOUT); return NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; } @@ -2712,13 +2712,13 @@ nc_ps_poll_session_io(struct nc_session *session, int io_timeout, time_t now_mon r = ssh_channel_poll_timeout(session->ti.libssh.channel, 0, 0); if (r == SSH_EOF) { sprintf(msg, "SSH channel unexpected EOF"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); ret = NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; } else if (r == SSH_ERROR) { sprintf(msg, "SSH channel poll error (%s)", ssh_get_error(session->ti.libssh.session)); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); ret = NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; } else if (!r) { /* no application data received */ @@ -2744,18 +2744,18 @@ nc_ps_poll_session_io(struct nc_session *session, int io_timeout, time_t now_mon if (r < 0) { sprintf(msg, "Poll failed (%s)", strerror(errno)); - session->status = NC_STATUS_INVALID; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); ret = NC_PSPOLL_ERROR; } else if (r > 0) { if (pfd.revents & (POLLHUP | POLLNVAL)) { sprintf(msg, "Communication socket unexpectedly closed"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); ret = NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; } else if (pfd.revents & POLLERR) { sprintf(msg, "Communication socket error"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); ret = NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; } else { ret = NC_PSPOLL_RPC; @@ -2777,18 +2777,18 @@ nc_ps_poll_session_io(struct nc_session *session, int io_timeout, time_t now_mon if (r < 0) { sprintf(msg, "Poll failed (%s)", strerror(errno)); - session->status = NC_STATUS_INVALID; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); ret = NC_PSPOLL_ERROR; } else if (r > 0) { if (pfd.revents & (POLLHUP | POLLNVAL)) { sprintf(msg, "Communication socket unexpectedly closed"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_DROPPED; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_DROPPED); ret = NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; } else if (pfd.revents & POLLERR) { sprintf(msg, "Communication socket error"); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); ret = NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; } else { ret = NC_PSPOLL_RPC; @@ -2827,7 +2827,7 @@ nc_ps_poll_sess(struct nc_ps_session *ps_session, time_t now_mono) switch (ps_session->state) { case NC_PS_STATE_NONE: - if (ps_session->session->status == NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(ps_session->session) == NC_STATUS_RUNNING) { /* session is fine, work with it, no configuration is accessed */ ps_session->state = NC_PS_STATE_BUSY; ret = nc_ps_poll_session_io(ps_session->session, NC_SESSION_LOCK_TIMEOUT, now_mono, msg); @@ -2855,7 +2855,7 @@ nc_ps_poll_sess(struct nc_ps_session *ps_session, time_t now_mono) } else { /* session is not fine, let the caller know */ ret = NC_PSPOLL_SESSION_TERM; - if (ps_session->session->term_reason != NC_SESSION_TERM_CLOSED) { + if (NC_SESSION_TERM_REASON_GET(ps_session->session) != NC_SESSION_TERM_CLOSED) { ret |= NC_PSPOLL_SESSION_ERROR; } ps_session->state = NC_PS_STATE_INVALID; @@ -2987,7 +2987,7 @@ nc_ps_poll(struct nc_pollsession *ps, int timeout, struct nc_session **session) ret = nc_server_recv_rpc_io(cur_session, timeout, &rpc); if (ret & (NC_PSPOLL_ERROR | NC_PSPOLL_BAD_RPC)) { /* error, do not send a reply */ - if (cur_session->status != NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(cur_session) != NC_STATUS_RUNNING) { ret |= NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR; cur_ps_session->state = NC_PS_STATE_INVALID; } else { @@ -3001,9 +3001,9 @@ nc_ps_poll(struct nc_pollsession *ps, int timeout, struct nc_session **session) /* process RPC and send a reply */ ret |= nc_server_send_reply_io(cur_session, timeout, rpc); - if (cur_session->status != NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(cur_session) != NC_STATUS_RUNNING) { ret |= NC_PSPOLL_SESSION_TERM; - if (!(cur_session->term_reason & (NC_SESSION_TERM_CLOSED | NC_SESSION_TERM_KILLED))) { + if (!(NC_SESSION_TERM_REASON_GET(cur_session) & (NC_SESSION_TERM_CLOSED | NC_SESSION_TERM_KILLED))) { ret |= NC_PSPOLL_SESSION_ERROR; } cur_ps_session->state = NC_PS_STATE_INVALID; @@ -3048,7 +3048,7 @@ nc_ps_clear(struct nc_pollsession *ps, int all, void (*data_free)(void *)) ps->last_event_session = 0; } else { for (i = 0; i < ps->session_count; ) { - if (ps->sessions[i]->session->status != NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(ps->sessions[i]->session) != NC_STATUS_RUNNING) { session = ps->sessions[i]->session; _nc_ps_del_session(ps, NULL, i); nc_session_free(session, data_free); @@ -3652,7 +3652,7 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) *session = nc_new_session(NC_SERVER, 0); NC_CHECK_ERRMEM_GOTO(!(*session), msgtype = NC_MSG_ERROR, cleanup); - (*session)->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(*session, NC_STATUS_STARTING); (*session)->ctx = (struct ly_ctx *)ctx; (*session)->flags = NC_SESSION_SHAREDCTX; (*session)->host = host; @@ -3725,7 +3725,7 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) (*session)->opts.server.last_rpc = ts_cur.tv_sec; nc_realtime_get(&ts_cur); (*session)->opts.server.session_start = ts_cur; - (*session)->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(*session, NC_STATUS_RUNNING); return msgtype; @@ -3879,7 +3879,7 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en /* create session */ *session = nc_new_session(NC_SERVER, 0); NC_CHECK_ERRMEM_GOTO(!(*session), close(sock); free(ip_host); msgtype = NC_MSG_ERROR, fail); - (*session)->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(*session, NC_STATUS_STARTING); (*session)->ctx = (struct ly_ctx *)ctx; (*session)->flags = NC_SESSION_SHAREDCTX | NC_SESSION_CALLHOME; (*session)->host = ip_host; @@ -3940,7 +3940,7 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en (*session)->opts.server.last_rpc = ts_cur.tv_sec; nc_realtime_get(&ts_cur); (*session)->opts.server.session_start = ts_cur; - (*session)->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(*session, NC_STATUS_RUNNING); return msgtype; @@ -4052,7 +4052,7 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat r = pthread_cond_clockwait(&session->opts.server.ch_cond, &session->opts.server.ch_lock, COMPAT_CLOCK_ID, &ts); if (!r) { /* we were woken up, something probably happened */ - if (session->status != NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) { break; } } else if (r != ETIMEDOUT) { @@ -4090,13 +4090,13 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat nc_timeouttime_get(&ts, 0); if (!nc_session_get_notif_status(session) && idle_timeout && (ts.tv_sec >= session->opts.server.last_rpc + idle_timeout)) { VRB(session, "Call Home client \"%s\": session idle timeout elapsed.", data->client_name); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_TIMEOUT; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_TIMEOUT); } - } while (session->status == NC_STATUS_RUNNING); + } while (NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING); /* broke out of the loop, but still holding the ch_lock */ - if (session->status == NC_STATUS_RUNNING) { + if (NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING) { /* thread is terminating but the session is still running, so just log it */ VRB(session, "Call Home client \"%s\" removed, but an established session will not be terminated.", data->client_name); diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index 1ef46b0d..fc97323e 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -518,7 +518,7 @@ nc_server_ssh_channel_subsys_check(struct nc_session *session, ssh_channel chann if (session->ti.libssh.channel == channel) { /* first channel requested */ - if (session->ti.libssh.next || (session->status != NC_STATUS_STARTING)) { + if (session->ti.libssh.next || (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { ERRINT; return -1; } @@ -551,7 +551,7 @@ nc_server_ssh_new_channel_session(struct nc_session *session, ssh_channel channe new_session = nc_new_session(NC_SERVER, 1); NC_CHECK_ERRMEM_RET(!new_session, NULL); - new_session->status = NC_STATUS_STARTING; + NC_SESSION_STATUS_SET(new_session, NC_STATUS_STARTING); new_session->ti_type = NC_TI_SSH; new_session->io_lock = session->io_lock; new_session->ti.libssh.channel = channel; @@ -2056,12 +2056,12 @@ nc_session_accept_ssh_channel(struct nc_session *orig_session, struct nc_session NC_CHECK_ARG_RET(orig_session, orig_session, session, NC_MSG_ERROR); - if ((orig_session->status == NC_STATUS_RUNNING) && (orig_session->ti_type == NC_TI_SSH) && + if ((NC_SESSION_STATUS_GET(orig_session) == NC_STATUS_RUNNING) && (orig_session->ti_type == NC_TI_SSH) && orig_session->ti.libssh.next) { for (new_session = orig_session->ti.libssh.next; new_session != orig_session; new_session = new_session->ti.libssh.next) { - if ((new_session->status == NC_STATUS_STARTING) && new_session->ti.libssh.channel && + if ((NC_SESSION_STATUS_GET(new_session) == NC_STATUS_STARTING) && new_session->ti.libssh.channel && (new_session->flags & NC_SESSION_SSH_SUBSYS_NETCONF)) { /* we found our session */ break; @@ -2090,7 +2090,7 @@ nc_session_accept_ssh_channel(struct nc_session *orig_session, struct nc_session new_session->opts.server.session_start = ts_cur; nc_timeouttime_get(&ts_cur, 0); new_session->opts.server.last_rpc = ts_cur.tv_sec; - new_session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(new_session, NC_STATUS_RUNNING); *session = new_session; return msgtype; @@ -2114,13 +2114,13 @@ nc_ps_accept_ssh_channel(struct nc_pollsession *ps, struct nc_session **session) for (i = 0; i < ps->session_count; ++i) { cur_session = ps->sessions[i]->session; - if ((cur_session->status == NC_STATUS_RUNNING) && (cur_session->ti_type == NC_TI_SSH) && + if ((NC_SESSION_STATUS_GET(cur_session) == NC_STATUS_RUNNING) && (cur_session->ti_type == NC_TI_SSH) && cur_session->ti.libssh.next) { /* an SSH session with more channels */ for (new_session = cur_session->ti.libssh.next; new_session != cur_session; new_session = new_session->ti.libssh.next) { - if ((new_session->status == NC_STATUS_STARTING) && new_session->ti.libssh.channel && + if ((NC_SESSION_STATUS_GET(new_session) == NC_STATUS_STARTING) && new_session->ti.libssh.channel && (new_session->flags & NC_SESSION_SSH_SUBSYS_NETCONF)) { /* we found our session */ break; @@ -2155,7 +2155,7 @@ nc_ps_accept_ssh_channel(struct nc_pollsession *ps, struct nc_session **session) new_session->opts.server.session_start = ts_cur; nc_timeouttime_get(&ts_cur, 0); new_session->opts.server.last_rpc = ts_cur.tv_sec; - new_session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(new_session, NC_STATUS_RUNNING); *session = new_session; return msgtype; diff --git a/src/session_server_ssh_auth_callback.c b/src/session_server_ssh_auth_callback.c index d9e2676f..7df33317 100644 --- a/src/session_server_ssh_auth_callback.c +++ b/src/session_server_ssh_auth_callback.c @@ -504,8 +504,8 @@ nc_server_ssh_cb_auth_common_setup(struct nc_server_ssh_cb_data *cb_data, const } else if (strcmp(user, session->username)) { /* changing username not allowed */ ERR(session, "User \"%s\" changed its username to \"%s\".", session->username, user); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); nc_server_ssh_auth_attempt_failed(session); return -1; } @@ -788,7 +788,7 @@ nc_server_ssh_cb_channel_open_request_session(ssh_session libssh_sess, void *use struct nc_ssh_channel_cb_data *channel_data; /* first channel request */ - if (!session->ti.libssh.channel && (session->status != NC_STATUS_STARTING)) { + if (!session->ti.libssh.channel && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { ERRINT; return NULL; } diff --git a/src/session_server_ssh_auth_message.c b/src/session_server_ssh_auth_message.c index e7ba6974..ec1c5749 100644 --- a/src/session_server_ssh_auth_message.c +++ b/src/session_server_ssh_auth_message.c @@ -324,7 +324,7 @@ nc_server_ssh_msg_channel_open(struct nc_session *session, ssh_message msg) /* first channel request */ if (!session->ti.libssh.channel) { - if (session->status != NC_STATUS_STARTING) { + if (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING) { ERRINT; return -1; } @@ -415,8 +415,8 @@ nc_server_ssh_msg_auth(struct nc_session *session, struct nc_server_ssh_opts *op } else if (strcmp(username, session->username)) { /* changing username not allowed */ ERR(session, "User \"%s\" changed its username to \"%s\".", session->username, username); - session->status = NC_STATUS_INVALID; - session->term_reason = NC_SESSION_TERM_OTHER; + NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); + NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_OTHER); nc_server_ssh_auth_attempt_failed(session); return 1; } @@ -593,11 +593,11 @@ nc_session_ssh_msg(struct nc_session *session, struct nc_server_ssh_opts *opts, } VRB(session, "Received an SSH message \"%s\" of subtype \"%s\".", str_type, str_subtype); - if (!session || (session->status == NC_STATUS_CLOSING) || (session->status == NC_STATUS_INVALID)) { + if (!session || (NC_SESSION_STATUS_GET(session) == NC_STATUS_CLOSING) || (NC_SESSION_STATUS_GET(session) == NC_STATUS_INVALID)) { /* "valid" situation if, for example, receiving some auth or channel request timeouted, * but we got it now, during session free */ VRB(session, "SSH message arrived on a %s session, the request will be denied.", - (session && session->status == NC_STATUS_CLOSING ? "closing" : "invalid")); + (session && NC_SESSION_STATUS_GET(session) == NC_STATUS_CLOSING ? "closing" : "invalid")); ssh_message_reply_default(msg); return 0; } diff --git a/tests/test_fd_comm.c b/tests/test_fd_comm.c index 8a9f77d1..11dbdc47 100644 --- a/tests/test_fd_comm.c +++ b/tests/test_fd_comm.c @@ -134,7 +134,7 @@ setup_sessions(void **state) /* create server session */ server_session = test_new_session(NC_SERVER); - server_session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(server_session, NC_STATUS_RUNNING); server_session->id = 1; server_session->ti_type = NC_TI_FD; server_session->ti.fd.in = sock[0]; @@ -144,7 +144,7 @@ setup_sessions(void **state) /* create client session */ client_session = test_new_session(NC_CLIENT); - client_session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(client_session, NC_STATUS_RUNNING); client_session->id = 1; client_session->ti_type = NC_TI_FD; client_session->ti.fd.in = sock[1]; diff --git a/tests/test_io.c b/tests/test_io.c index d66ba84a..55d1ea96 100644 --- a/tests/test_io.c +++ b/tests/test_io.c @@ -64,7 +64,7 @@ setup_write(void **state) assert_return_code(pipe(pipes), errno); - w->session->status = NC_STATUS_RUNNING; + NC_SESSION_STATUS_SET(w->session, NC_STATUS_RUNNING); w->session->version = NC_PROT_VERSION_10; w->session->opts.client.msgid = 999; w->session->ti_type = NC_TI_FD; From daece9677ee416ab90f6df463d62cc1335aa7a01 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Wed, 2 Sep 2026 17:02:13 +0200 Subject: [PATCH 3/7] session server BUGFIX move the CH thread flag out of flags ThreadSanitizer reports a data race between the Call Home thread clearing NC_SESSION_CH_THREAD in nc_session.flags and a poll thread reading NC_SESSION_CALLHOME from the same byte in nc_ps_poll_session_io(). The flag was documented as protected by ch_lock, but a read-modify-write of a byte shared with flags that nobody locks cannot be made safe by any lock. The flag also could not keep that contract in the first place: two error paths had to write it without the lock, and two more returned without clearing it at all, leaving the session marked as owned by a Call Home thread that had already exited. nc_session_free() then burned its whole timeout waiting for nobody. Replace it with a dedicated atomic, nc_session.opts.server .ch_thread_active, so it can be cleared on every path the thread leaves by, and let nc_session_free() poll it instead of waiting on ch_cond. That drops the ch_lock dependency of the wait, so freeing a Call Home session whose lock could not be acquired now waits for the thread properly instead of logging an error and racing on. ch_cond keeps its other direction, nc_session_free() telling the Call Home thread the session is closing, so the signal stays. Fixes #635 --- src/session.c | 60 +++++++++++++------------------------------- src/session_p.h | 16 ++++++++++-- src/session_server.c | 33 +++++++----------------- 3 files changed, 40 insertions(+), 69 deletions(-) diff --git a/src/session.c b/src/session.c index e05d823d..577e46ab 100644 --- a/src/session.c +++ b/src/session.c @@ -1086,31 +1086,13 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) int r, i, rpc_locked = 0, ch_locked = 0; int multisession = 0; /* flag for more NETCONF sessions on a single SSH session */ struct timespec ts; - NC_STATUS status; if (!session) { return; } - if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { - /* CH LOCK, continue on error */ - if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) == 1) { - ch_locked = 1; - } - } - - /* store status, so we can check if this session is already closing */ - status = NC_SESSION_STATUS_GET(session); - - if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { - /* CH UNLOCK */ - if (ch_locked) { - /* only if we locked it */ - nc_mutex_unlock(&session->opts.server.ch_lock, __func__); - } - } - - if (status == NC_STATUS_CLOSING) { + /* check whether this session is already closing */ + if (NC_SESSION_STATUS_GET(session) == NC_STATUS_CLOSING) { return; } @@ -1160,8 +1142,7 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) } if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { - /* CH LOCK */ - ch_locked = 0; + /* CH LOCK, continue on error */ if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) == 1) { ch_locked = 1; } @@ -1170,34 +1151,27 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) /* mark session for closing */ NC_SESSION_STATUS_SET(session, NC_STATUS_CLOSING); - if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CH_THREAD)) { - /* signaling a condition does not require its mutex to be held */ + if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { + /* wake up the Call Home thread so that it learns the session is closing, done while holding + * ch_lock (if we got it) so that a thread about to wait on the condition cannot miss it */ pthread_cond_signal(&session->opts.server.ch_cond); - if (ch_locked) { - nc_timeouttime_get(&ts, NC_SESSION_FREE_LOCK_TIMEOUT); - - /* wait for CH thread to actually wake up and terminate */ - r = 0; - while (!r && (session->flags & NC_SESSION_CH_THREAD)) { - r = pthread_cond_clockwait(&session->opts.server.ch_cond, &session->opts.server.ch_lock, COMPAT_CLOCK_ID, &ts); - } - if (r) { - ERR(session, "Waiting for Call Home thread failed (%s).", strerror(r)); - } - } else { - /* waiting on a condition requires its mutex to be held by the caller, so there is no - * way to wait for the Call Home thread without ch_lock */ - ERR(session, "Freeing a Call Home session without its lock, not waiting for its thread."); - } - } - - if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { /* CH UNLOCK */ if (ch_locked) { /* only if we locked it */ nc_mutex_unlock(&session->opts.server.ch_lock, __func__); } + + /* wait for the Call Home thread to stop using the session, it needs ch_lock to get there + * so this must not be done while holding it */ + nc_timeouttime_get(&ts, NC_SESSION_FREE_LOCK_TIMEOUT); + while (ATOMIC_LOAD_ACQUIRE(session->opts.server.ch_thread_active)) { + if (nc_timeouttime_cur_diff(&ts) < 1) { + ERR(session, "Waiting for the Call Home thread timed out."); + break; + } + usleep(NC_TIMEOUT_STEP); + } } /* transport implementation cleanup */ diff --git a/src/session_p.h b/src/session_p.h index 611ec405..284cbb54 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -1090,7 +1090,7 @@ struct nc_session { /* shared flags */ #define NC_SESSION_SHAREDCTX 0x01 /**< context is shared */ #define NC_SESSION_CALLHOME 0x02 /**< session is Call Home and ch_lock is initialized */ -#define NC_SESSION_CH_THREAD 0x04 /**< protected by ch_lock */ +/* 0x04 is free, it used to be NC_SESSION_CH_THREAD, see ::nc_session.opts.server.ch_thread_active */ /* client flags */ #define NC_SESSION_CLIENT_NOT_STRICT 0x08 /**< some server modules failed to load so the data from @@ -1126,7 +1126,19 @@ struct nc_session { rpc_cond and rpc_lock) */ pthread_mutex_t ch_lock; /**< Call Home thread lock */ - pthread_cond_t ch_cond; /**< Call Home thread condition */ + pthread_cond_t ch_cond; /**< Call Home thread condition, signalled by ::nc_session_free() + to tell the Call Home thread the session is closing */ + + /** + * @brief Non-zero while a Call Home thread is using this session. + * + * Set before the session is given to the user and cleared once the Call Home thread is done + * with it, which ::nc_session_free() waits for before tearing the session down. Deliberately + * not protected by @p ch_lock: the Call Home thread has to be able to clear it on the error + * paths where it failed to acquire the lock. Cleared with a release store so that everything + * the Call Home thread did to the session is published to ::nc_session_free(). + */ + ATOMIC_T ch_thread_active; /** * @brief Configuration generation pinned for the duration of the transport handshake. diff --git a/src/session_server.c b/src/session_server.c index 18a3309d..e2d4053d 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -4009,29 +4009,13 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat uint32_t idle_timeout; struct timespec ts; - /* CH LOCK */ - if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) != 1) { - /* the session has not been given to the user yet, so it is still ours to free */ - nc_session_free(session, NULL); - data->release_ctx_cb(data->ctx_cb_data); - return -1; - } - - session->flags |= NC_SESSION_CH_THREAD; - - /* CH UNLOCK */ - nc_mutex_unlock(&session->opts.server.ch_lock, __func__); + /* claim the session, ::nc_session_free() waits for this to be cleared */ + ATOMIC_STORE_RELAXED(session->opts.server.ch_thread_active, 1); /* give the session to the user */ if (data->new_session_cb(data->client_name, session, data->new_session_cb_data)) { - /* CH LOCK, continue on error */ - nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__); - - /* something is wrong, free the session */ - session->flags &= ~NC_SESSION_CH_THREAD; - - /* CH UNLOCK */ - nc_mutex_unlock(&session->opts.server.ch_lock, __func__); + /* something is wrong, we are done with the session */ + ATOMIC_STORE_RELEASE(session->opts.server.ch_thread_active, 0); /* session terminated, free it and release its context */ nc_session_free(session, NULL); @@ -4041,6 +4025,7 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat /* CH LOCK */ if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) != 1) { + ATOMIC_STORE_RELEASE(session->opts.server.ch_thread_active, 0); return -1; } @@ -4079,6 +4064,7 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat /* CH LOCK */ if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) != 1) { + ATOMIC_STORE_RELEASE(session->opts.server.ch_thread_active, 0); return -1; } @@ -4102,13 +4088,12 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat data->client_name); } - /* signal to nc_session_free() that CH thread is terminating */ - session->flags &= ~NC_SESSION_CH_THREAD; - pthread_cond_signal(&session->opts.server.ch_cond); - /* CH UNLOCK */ nc_mutex_unlock(&session->opts.server.ch_lock, __func__); + /* release the session, ::nc_session_free() may tear it down as soon as this is observed */ + ATOMIC_STORE_RELEASE(session->opts.server.ch_thread_active, 0); + return rc; } From 142a20da8f3d500d407ed908f9d44bae866c05e4 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Thu, 3 Sep 2026 14:12:24 +0200 Subject: [PATCH 4/7] session server BUGFIX check session status before CH cond wait The Call Home thread entered pthread_cond_clockwait() without ever checking the session status first, so the signal nc_session_free() sends to say the session is closing was lost whenever it fired before the thread reached the wait. The thread then slept for the full NC_CH_THREAD_IDLE_TIMEOUT_SLEEP, and since ETIMEDOUT skips the status check it took a config lock and a ch_lock re-acquisition on top of that before releasing the session. nc_session_free() waits only NC_SESSION_FREE_LOCK_TIMEOUT, which is the same 1000 ms, so it could give up first and go on to destroy ch_cond and ch_lock and free the session while the thread was still blocked on them. Turn the loop into a while so that the status is tested under ch_lock before the first wait, which is the lock nc_session_free() sets it under. --- src/session_server.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/session_server.c b/src/session_server.c index e2d4053d..c87ee7a1 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -4029,8 +4029,10 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat return -1; } - /* entering the loop with locked ch_lock */ - do { + /* entering the loop with locked ch_lock, the status must be checked before waiting on the + * condition, otherwise a session closed before we got the lock is missed and we sleep until + * the wait times out, by which point ::nc_session_free() may have given up on us */ + while (NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING) { nc_timeouttime_get(&ts, NC_CH_THREAD_IDLE_TIMEOUT_SLEEP); /* CH COND WAIT */ @@ -4079,8 +4081,8 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID); NC_SESSION_TERM_REASON_SET(session, NC_SESSION_TERM_TIMEOUT); } - } while (NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING); - /* broke out of the loop, but still holding the ch_lock */ + } + /* left the loop, but still holding the ch_lock */ if (NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING) { /* thread is terminating but the session is still running, so just log it */ From 6f80c0ef995ecc02eddb2b56efe0810d65b27270 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Thu, 3 Sep 2026 14:12:31 +0200 Subject: [PATCH 5/7] session server BUGFIX report abnormally terminated sessions nc_ps_poll() tested the termination reason with a bitwise AND against NC_SESSION_TERM_CLOSED | NC_SESSION_TERM_KILLED, but NC_SESSION_TERM_REASON is a sequential enum, not a bitmask. That mask is 3, so DROPPED (3), BADHELLO (5) and OTHER (6) all tested non-zero and the session was reported as a clean close without NC_PSPOLL_SESSION_ERROR. Compare against both reasons explicitly, the way nc_ps_poll_sess() already does. --- src/session_server.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/session_server.c b/src/session_server.c index c87ee7a1..8f824a26 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -3003,7 +3003,8 @@ nc_ps_poll(struct nc_pollsession *ps, int timeout, struct nc_session **session) ret |= nc_server_send_reply_io(cur_session, timeout, rpc); if (NC_SESSION_STATUS_GET(cur_session) != NC_STATUS_RUNNING) { ret |= NC_PSPOLL_SESSION_TERM; - if (!(NC_SESSION_TERM_REASON_GET(cur_session) & (NC_SESSION_TERM_CLOSED | NC_SESSION_TERM_KILLED))) { + if ((NC_SESSION_TERM_REASON_GET(cur_session) != NC_SESSION_TERM_CLOSED) && + (NC_SESSION_TERM_REASON_GET(cur_session) != NC_SESSION_TERM_KILLED)) { ret |= NC_PSPOLL_SESSION_ERROR; } cur_ps_session->state = NC_PS_STATE_INVALID; From 64ad98d37ad2913f830e967804e306f99c8a1e0e Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Thu, 3 Sep 2026 14:12:37 +0200 Subject: [PATCH 6/7] session server DOC clarify the IO lock contract "Send a reply acquiring IO lock as needed. Session RPC lock must be held!" reads as if the caller had to hold the IO lock. It describes what the function does internally, the caller only has to hold the RPC lock; say so. Same for nc_ps_poll_session_io(). --- src/session_server.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/session_server.c b/src/session_server.c index 8f824a26..fee185c2 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -2526,8 +2526,10 @@ nc_server_notif_send(struct nc_session *session, struct nc_server_notif *notif, } /** - * @brief Send a reply acquiring IO lock as needed. - * Session RPC lock must be held! + * @brief Send a reply to an RPC. + * + * The session IO lock is acquired internally, the caller must not hold it. The caller must, + * however, hold the session RPC lock. * * @param[in] session Session to use. * @param[in] io_timeout Timeout to use for acquiring IO lock. @@ -2634,8 +2636,10 @@ nc_ps_ssh_find_new_channel(struct nc_session *session) #endif /* NC_ENABLED_SSH_TLS */ /** - * @brief Poll a session from pspoll acquiring IO lock as needed. - * Session must be running and session RPC lock held! + * @brief Poll a session from pspoll. + * + * The session IO lock is acquired internally, the caller must not hold it. The caller must, + * however, hold the session RPC lock and the session must be running. * * @param[in] session Session to use. * @param[in] io_timeout Timeout to use for acquiring IO lock. From 83b28167cda202044ab702f40e6233d2e4de0fbf Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Thu, 3 Sep 2026 14:27:21 +0200 Subject: [PATCH 7/7] session REFACTOR read the session status only once per check Several conditions read nc_session.status twice, which are two separate atomic loads. Another thread can change the status between them, so the two comparisons can be evaluated against different values and reach a conclusion that was never true at any single point in time. nc_session_ssh_msg() showed this best: it tested the status twice to decide the session is not usable and then read it a third time to name it in the log, so the message could say "invalid" about a session the condition had matched as "closing". Take one snapshot into a local and compare that. Checks that are meant to observe a change, such as the second one in nc_write_msg_io() after the message has been written, keep re-reading. --- src/io.c | 27 ++++++++++++++++++++------- src/session.c | 6 ++++-- src/session_server.c | 5 +++-- src/session_server_ssh_auth_message.c | 6 ++++-- 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/io.c b/src/io.c index 9c5e0b55..762e2223 100644 --- a/src/io.c +++ b/src/io.c @@ -76,11 +76,13 @@ nc_read(struct nc_session *session, char *buf, uint32_t count, uint32_t inact_ti ssize_t r = -1; int fd, interrupted; struct timespec ts_inact_timeout; + NC_STATUS status; assert(session); assert(buf); - if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { return -1; } @@ -265,13 +267,15 @@ nc_read_msg_io(struct nc_session *session, int io_timeout, int passing_io_lock, char *frame_size_buf = NULL; uint32_t inact_timeout, frame_buf_len, chunk_len, buf_used = 0; struct timespec ts_act_timeout; + NC_STATUS status; assert(session && buf && buf_len); /* use timeout in milliseconds instead seconds */ inact_timeout = NC_READ_INACT_TIMEOUT * 1000; - if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { ERR(session, "Invalid session to read from."); ret = -1; goto cleanup; @@ -385,8 +389,10 @@ nc_read_poll(struct nc_session *session, int io_timeout) { int ret = -2; struct pollfd fds; + NC_STATUS status; - if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { ERR(session, "Invalid session to poll."); return -1; } @@ -484,11 +490,13 @@ nc_read_msg_poll_io(struct nc_session *session, int io_timeout, struct ly_in **m int ret; uint32_t buf_len = 0; char *buf = NULL; + NC_STATUS status; assert(msg); *msg = NULL; - if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { ERR(session, "Invalid session to read from."); return -1; } @@ -587,8 +595,10 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) { int c, fd, interrupted; uint32_t written = 0; + NC_STATUS status; - if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { return -1; } @@ -851,10 +861,12 @@ nc_write_msg_io(struct nc_session *session, int io_timeout, int type, ...) const char **capabilities; uint32_t *sid = NULL, i, wd = 0, str_len; LY_ERR lyrc; + NC_STATUS status; assert(session); - if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { ERR(session, "Invalid session to write to."); return NC_MSG_ERROR; } @@ -1078,7 +1090,8 @@ nc_write_msg_io(struct nc_session *session, int io_timeout, int type, ...) /* flush message */ nc_write_clb((void *)&arg, NULL, 0, 0); - if ((NC_SESSION_STATUS_GET(session) != NC_STATUS_RUNNING) && (NC_SESSION_STATUS_GET(session) != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { /* error was already written */ ret = NC_MSG_ERROR; } else { diff --git a/src/session.c b/src/session.c index 577e46ab..02d4e9a2 100644 --- a/src/session.c +++ b/src/session.c @@ -1086,6 +1086,7 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) int r, i, rpc_locked = 0, ch_locked = 0; int multisession = 0; /* flag for more NETCONF sessions on a single SSH session */ struct timespec ts; + NC_STATUS status; if (!session) { return; @@ -1129,9 +1130,10 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) /* notify the peer that we're closing the session, either if: * - session running - normal disconnect from client * - session invalid - client disconnected from a Call Home session */ - if ((NC_SESSION_STATUS_GET(session) == NC_STATUS_RUNNING) || + status = NC_SESSION_STATUS_GET(session); + if ((status == NC_STATUS_RUNNING) || ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME) && - (NC_SESSION_STATUS_GET(session) == NC_STATUS_INVALID) && (NC_SESSION_TERM_REASON_GET(session) == NC_SESSION_TERM_CLOSED))) { + (status == NC_STATUS_INVALID) && (NC_SESSION_TERM_REASON_GET(session) == NC_SESSION_TERM_CLOSED))) { if (session->side == NC_CLIENT) { /* graceful close: + transport shutdown indication */ nc_session_free_client_close_graceful(session); diff --git a/src/session_server.c b/src/session_server.c index fee185c2..688e5dc1 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -2889,6 +2889,7 @@ nc_ps_poll(struct nc_pollsession *ps, int timeout, struct nc_session **session) struct nc_session *cur_session; struct nc_ps_session *cur_ps_session; struct nc_server_rpc *rpc = NULL; + NC_SESSION_TERM_REASON term_reason; NC_CHECK_ARG_RET(NULL, ps, NC_PSPOLL_ERROR); @@ -3007,8 +3008,8 @@ nc_ps_poll(struct nc_pollsession *ps, int timeout, struct nc_session **session) ret |= nc_server_send_reply_io(cur_session, timeout, rpc); if (NC_SESSION_STATUS_GET(cur_session) != NC_STATUS_RUNNING) { ret |= NC_PSPOLL_SESSION_TERM; - if ((NC_SESSION_TERM_REASON_GET(cur_session) != NC_SESSION_TERM_CLOSED) && - (NC_SESSION_TERM_REASON_GET(cur_session) != NC_SESSION_TERM_KILLED)) { + term_reason = NC_SESSION_TERM_REASON_GET(cur_session); + if ((term_reason != NC_SESSION_TERM_CLOSED) && (term_reason != NC_SESSION_TERM_KILLED)) { ret |= NC_PSPOLL_SESSION_ERROR; } cur_ps_session->state = NC_PS_STATE_INVALID; diff --git a/src/session_server_ssh_auth_message.c b/src/session_server_ssh_auth_message.c index ec1c5749..56b82908 100644 --- a/src/session_server_ssh_auth_message.c +++ b/src/session_server_ssh_auth_message.c @@ -480,6 +480,7 @@ nc_session_ssh_msg(struct nc_session *session, struct nc_server_ssh_opts *opts, { const char *str_type, *str_subtype = NULL; int subtype, type, local_users_supported; + NC_STATUS status; type = ssh_message_type(msg); subtype = ssh_message_subtype(msg); @@ -593,11 +594,12 @@ nc_session_ssh_msg(struct nc_session *session, struct nc_server_ssh_opts *opts, } VRB(session, "Received an SSH message \"%s\" of subtype \"%s\".", str_type, str_subtype); - if (!session || (NC_SESSION_STATUS_GET(session) == NC_STATUS_CLOSING) || (NC_SESSION_STATUS_GET(session) == NC_STATUS_INVALID)) { + status = session ? NC_SESSION_STATUS_GET(session) : NC_STATUS_ERR; + if (!session || (status == NC_STATUS_CLOSING) || (status == NC_STATUS_INVALID)) { /* "valid" situation if, for example, receiving some auth or channel request timeouted, * but we got it now, during session free */ VRB(session, "SSH message arrived on a %s session, the request will be denied.", - (session && NC_SESSION_STATUS_GET(session) == NC_STATUS_CLOSING ? "closing" : "invalid")); + (status == NC_STATUS_CLOSING) ? "closing" : "invalid"); ssh_message_reply_default(msg); return 0; }