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) \ { \ diff --git a/src/io.c b/src/io.c index 7c499506..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 ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != NC_STATUS_STARTING)) { return -1; } @@ -110,14 +112,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 +133,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 +168,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 { @@ -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 ((session->status != NC_STATUS_RUNNING) && (session->status != 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 ((session->status != NC_STATUS_RUNNING) && (session->status != 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; } @@ -403,13 +409,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 +460,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; } } @@ -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 ((session->status != NC_STATUS_RUNNING) && (session->status != 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,16 +595,18 @@ nc_write(struct nc_session *session, const void *buf, uint32_t count) { int c, fd, interrupted; uint32_t written = 0; + NC_STATUS status; - if ((session->status != NC_STATUS_RUNNING) && (session->status != NC_STATUS_STARTING)) { + status = NC_SESSION_STATUS_GET(session); + if ((status != NC_STATUS_RUNNING) && (status != 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 +634,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); @@ -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 ((session->status != NC_STATUS_RUNNING) && (session->status != 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 ((session->status != NC_STATUS_RUNNING) && (session->status != 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/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..02d4e9a2 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; } @@ -1092,25 +1092,8 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) 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 = session->status; - - 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; } @@ -1147,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 ((session->status == NC_STATUS_RUNNING) || + status = NC_SESSION_STATUS_GET(session); + if ((status == NC_STATUS_RUNNING) || ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME) && - (session->status == NC_STATUS_INVALID) && (session->term_reason == 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); @@ -1160,44 +1144,36 @@ 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; } } /* 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 */ + 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_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..284cbb54 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 */ @@ -1086,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 @@ -1122,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. @@ -1152,6 +1168,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..688e5dc1 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); } } @@ -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. @@ -2598,8 +2600,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 +2624,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; } @@ -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. @@ -2666,8 +2670,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 +2716,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 +2748,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 +2781,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 +2831,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 +2859,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; @@ -2885,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); @@ -2987,7 +2992,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 +3006,10 @@ 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))) { + 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; @@ -3048,7 +3054,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 +3658,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 +3731,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 +3885,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 +3946,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; @@ -4009,29 +4015,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,18 +4031,21 @@ 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; } - /* 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 */ 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) { @@ -4079,6 +4072,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; } @@ -4090,25 +4084,24 @@ 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); - /* broke out of the loop, but still holding the ch_lock */ + } + /* left 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); } - /* 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; } 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..56b82908 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; } @@ -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 || (session->status == NC_STATUS_CLOSING) || (session->status == 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 && session->status == NC_STATUS_CLOSING ? "closing" : "invalid")); + (status == 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;