server BUGFIX nc_ps_poll deadlock due to multi-thread conflict
diff --git a/src/session_p.h b/src/session_p.h
index 333eb09..a52f396 100644
--- a/src/session_p.h
+++ b/src/session_p.h
@@ -424,12 +424,14 @@
NC_PS_STATE_INVALID /**< session is invalid and was already returned by another poll */
};
+struct nc_ps_session {
+ struct nc_session *session;
+ enum nc_ps_session_state state;
+};
+
/* ACCESS locked */
struct nc_pollsession {
- struct {
- struct nc_session *session;
- enum nc_ps_session_state state;
- } *sessions;
+ struct nc_ps_session **sessions;
uint16_t session_count;
uint16_t last_event_session;
diff --git a/src/session_server.c b/src/session_server.c
index bd610d9..b584401 100644
--- a/src/session_server.c
+++ b/src/session_server.c
@@ -816,6 +816,8 @@
API void
nc_ps_free(struct nc_pollsession *ps)
{
+ uint16_t i;
+
if (!ps) {
return;
}
@@ -824,6 +826,10 @@
ERR("FATAL: Freeing a pollsession structure that is currently being worked with!");
}
+ for (i = 0; i < ps->session_count; i++) {
+ free(ps->sessions[i]);
+ }
+
free(ps->sessions);
pthread_mutex_destroy(&ps->lock);
pthread_cond_destroy(&ps->cond);
@@ -857,8 +863,16 @@
nc_ps_unlock(ps, q_id, __func__);
return -1;
}
- ps->sessions[ps->session_count - 1].session = session;
- ps->sessions[ps->session_count - 1].state = NC_PS_STATE_NONE;
+ ps->sessions[ps->session_count - 1] = calloc(1, sizeof **ps->sessions);
+ if (!ps->sessions[ps->session_count - 1]) {
+ ERRMEM;
+ --ps->session_count;
+ /* UNLOCK */
+ nc_ps_unlock(ps, q_id, __func__);
+ return -1;
+ }
+ ps->sessions[ps->session_count - 1]->session = session;
+ ps->sessions[ps->session_count - 1]->state = NC_PS_STATE_NONE;
/* UNLOCK */
return nc_ps_unlock(ps, q_id, __func__);
@@ -874,12 +888,14 @@
goto remove;
}
for (i = 0; i < ps->session_count; ++i) {
- if (ps->sessions[i].session == session) {
+ if (ps->sessions[i]->session == session) {
remove:
--ps->session_count;
- if (i < ps->session_count) {
+ if (i <= ps->session_count) {
+ free(ps->sessions[i]);
ps->sessions[i] = ps->sessions[ps->session_count];
- } else if (!ps->session_count) {
+ }
+ if (!ps->session_count) {
free(ps->sessions);
ps->sessions = NULL;
}
@@ -936,8 +952,8 @@
}
for (i = 0; i < ps->session_count; ++i) {
- if (ps->sessions[i].session->id == sid) {
- ret = ps->sessions[i].session;
+ if (ps->sessions[i]->session->id == sid) {
+ ret = ps->sessions[i]->session;
break;
}
}
@@ -1314,6 +1330,7 @@
char msg[256];
struct timespec ts_timeout, ts_cur;
struct nc_session *cur_session;
+ struct nc_ps_session *cur_ps_session;
struct nc_server_rpc *rpc = NULL;
if (!ps) {
@@ -1347,7 +1364,8 @@
i = j = ps->last_event_session + 1;
}
do {
- cur_session = ps->sessions[i].session;
+ cur_ps_session = ps->sessions[i];
+ cur_session = cur_ps_session->session;
/* SESSION LOCK */
r = nc_session_lock(cur_session, 0, __func__);
@@ -1355,27 +1373,27 @@
ret = NC_PSPOLL_ERROR;
} else if (r == 1) {
/* no one else is currently working with the session, so we can, otherwise skip it */
- if (ps->sessions[i].state == NC_PS_STATE_NONE) {
+ if (cur_ps_session->state == NC_PS_STATE_NONE) {
if (cur_session->status == NC_STATUS_RUNNING) {
/* session is fine, work with it */
- ps->sessions[i].state = NC_PS_STATE_BUSY;
+ cur_ps_session->state = NC_PS_STATE_BUSY;
ret = nc_ps_poll_session(cur_session, ts_cur.tv_sec, msg);
switch (ret) {
case NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR:
ERR("Session %u: %s.", cur_session->id, msg);
- ps->sessions[i].state = NC_PS_STATE_INVALID;
+ cur_ps_session->state = NC_PS_STATE_INVALID;
break;
case NC_PSPOLL_ERROR:
ERR("Session %u: %s.", cur_session->id, msg);
- ps->sessions[i].state = NC_PS_STATE_NONE;
+ cur_ps_session->state = NC_PS_STATE_NONE;
break;
case NC_PSPOLL_TIMEOUT:
#ifdef NC_ENABLED_SSH
case NC_PSPOLL_SSH_CHANNEL:
case NC_PSPOLL_SSH_MSG:
#endif
- ps->sessions[i].state = NC_PS_STATE_NONE;
+ cur_ps_session->state = NC_PS_STATE_NONE;
break;
case NC_PSPOLL_RPC:
/* let's keep the state busy, we are not done with this session */
@@ -1387,9 +1405,9 @@
if (cur_session->term_reason != NC_SESSION_TERM_CLOSED) {
ret |= NC_PSPOLL_SESSION_ERROR;
}
- ps->sessions[i].state = NC_PS_STATE_INVALID;
+ cur_ps_session->state = NC_PS_STATE_INVALID;
}
- } else if (ps->sessions[i].state == NC_PS_STATE_BUSY) {
+ } else if (cur_ps_session->state == NC_PS_STATE_BUSY) {
/* it definitely should not be busy because we have the lock */
ERRINT;
}
@@ -1456,9 +1474,9 @@
if (ret & (NC_PSPOLL_ERROR | NC_PSPOLL_BAD_RPC)) {
if (cur_session->status != NC_STATUS_RUNNING) {
ret |= NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR;
- ps->sessions[i].state = NC_PS_STATE_INVALID;
+ cur_ps_session->state = NC_PS_STATE_INVALID;
} else {
- ps->sessions[i].state = NC_PS_STATE_NONE;
+ cur_ps_session->state = NC_PS_STATE_NONE;
}
} else {
cur_session->opts.server.last_rpc = time(NULL);
@@ -1472,9 +1490,9 @@
if (!(cur_session->term_reason & (NC_SESSION_TERM_CLOSED | NC_SESSION_TERM_KILLED))) {
ret |= NC_PSPOLL_SESSION_ERROR;
}
- ps->sessions[i].state = NC_PS_STATE_INVALID;
+ cur_ps_session->state = NC_PS_STATE_INVALID;
} else {
- ps->sessions[i].state = NC_PS_STATE_NONE;
+ cur_ps_session->state = NC_PS_STATE_NONE;
}
}
@@ -1504,7 +1522,8 @@
if (all) {
for (i = 0; i < ps->session_count; i++) {
- nc_session_free(ps->sessions[i].session, data_free);
+ nc_session_free(ps->sessions[i]->session, data_free);
+ free(ps->sessions[i]);
}
free(ps->sessions);
ps->sessions = NULL;
@@ -1512,8 +1531,8 @@
ps->last_event_session = 0;
} else {
for (i = 0; i < ps->session_count; ) {
- if (ps->sessions[i].session->status != NC_STATUS_RUNNING) {
- session = ps->sessions[i].session;
+ if (ps->sessions[i]->session->status != NC_STATUS_RUNNING) {
+ session = ps->sessions[i]->session;
_nc_ps_del_session(ps, NULL, i);
nc_session_free(session, data_free);
continue;
diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c
index 7187328..0881c5a 100644
--- a/src/session_server_ssh.c
+++ b/src/session_server_ssh.c
@@ -1540,7 +1540,7 @@
}
for (i = 0; i < ps->session_count; ++i) {
- cur_session = ps->sessions[i].session;
+ cur_session = ps->sessions[i]->session;
if ((cur_session->status == NC_STATUS_RUNNING) && (cur_session->ti_type == NC_TI_LIBSSH)
&& cur_session->ti.libssh.next) {
/* an SSH session with more channels */