fixed possible timer timeout/open reply message racing conditions; removed conn reset and reopen as unsafe

3 files changed, 119 insertions, 88 deletions

diff --git a/ecp/src/ecp/core.c b/ecp/src/ecp/core.c
index bb3309c..d111a97 100644
--- a/ecp/src/ecp/core.c
+++ b/ecp/src/ecp/core.c
@@ -928,31 +928,11 @@ int ecp_conn_init_outb(ECPConnection *conn) {
     ECPDHKey key;
     int rv;
 
-    conn->key_curr = 0;
-
     rv = ecp_dhkey_gen(&key);
     if (rv) return rv;
 
-    rv = conn_dhkey_new(conn, conn->key_curr, &key);
-    if (rv) return rv;
-
-    return ECP_OK;
-}
-
-int ecp_conn_reset(ECPConnection *conn) {
-    if (conn->flags) return ECP_ERR;
-
-    conn->key_curr = ECP_ECDH_IDX_INV;
-    conn->key_next = ECP_ECDH_IDX_INV;
-    conn->rkey_curr = ECP_ECDH_IDX_INV;
-    memset(&conn->key, 0, sizeof(conn->key));
-    memset(&conn->rkey, 0, sizeof(conn->rkey));
-    memset(&conn->shkey, 0, sizeof(conn->shkey));
-    arc4random_buf(&conn->nonce_out, sizeof(conn->nonce_out));
-    conn->access_ts = 0;
-    conn->keyx_ts = 0;
-    conn->nonce_in = 0;
-    conn->nonce_map = 0;
+    conn->key_curr = 0;
+    conn->key[conn->key_curr] = key;
 
     return ECP_OK;
 }
@@ -1066,30 +1046,6 @@ int ecp_conn_create_outb(ECPConnection *conn, ECPConnection *parent, ECPNode *no
     return ECP_OK;
 }
 
-int ecp_conn_reset_outb(ECPConnection *conn) {
-    int rv = ECP_OK;
-
-    ecp_conn_remove(conn);
-
-#ifdef ECP_WITH_PTHREAD
-    pthread_mutex_lock(&conn->mutex);
-#endif
-
-    if (_ecp_conn_is_open(conn)) rv = ECP_ERR;
-    if (!rv) rv = ecp_conn_reset(conn);
-    if (!rv) rv = ecp_conn_init_outb(conn);
-
-#ifdef ECP_WITH_PTHREAD
-    pthread_mutex_unlock(&conn->mutex);
-#endif
-    if (rv) return rv;
-
-    rv = ecp_conn_insert(conn);
-    if (rv) return rv;
-
-    return ECP_OK;
-}
-
 void ecp_conn_destroy(ECPConnection *conn) {
 #ifdef ECP_WITH_VCONN
     if (conn->parent) {
@@ -1171,6 +1127,30 @@ int ecp_conn_insert_gc(ECPConnection *conn) {
     return rv;
 }
 
+int _ecp_conn_remove(ECPConnection *conn) {
+    ECPSocket *sock = conn->sock;
+    int rv = ECP_OK;
+
+#ifdef ECP_WITH_PTHREAD
+    pthread_mutex_lock(&sock->conn_table.mutex);
+    pthread_mutex_lock(&conn->mutex);
+#endif
+
+    if (!_ecp_conn_is_reg(conn)) rv = ECP_ERR_CLOSED;
+    if (!rv && _ecp_conn_is_open(conn)) rv = ECP_ERR_BUSY;
+    if (!rv) {
+        conn_table_remove(conn);
+        _ecp_conn_clr_reg(conn);
+    }
+
+#ifdef ECP_WITH_PTHREAD
+    pthread_mutex_unlock(&conn->mutex);
+    pthread_mutex_unlock(&sock->conn_table.mutex);
+#endif
+
+    return rv;
+}
+
 void ecp_conn_remove(ECPConnection *conn) {
     ECPSocket *sock = conn->sock;
 
@@ -1268,15 +1248,6 @@ int _ecp_conn_open(ECPConnection *conn, ECPConnection *parent, ECPNode *node, in
     return rv;
 }
 
-int _ecp_conn_reopen(ECPConnection *conn, int retry) {
-    int rv;
-
-    rv = ecp_conn_reset_outb(conn);
-    if (rv) return rv;
-
-    return ecp_send_init_req(conn, retry);
-}
-
 int ecp_conn_open(ECPConnection *conn, ECPNode *node) {
     int rv;
 
@@ -1291,13 +1262,6 @@ int ecp_conn_try_open(ECPConnection *conn, ECPNode *node) {
     return rv;
 }
 
-int ecp_conn_reopen(ECPConnection *conn) {
-    int rv;
-
-    rv = _ecp_conn_reopen(conn, 1);
-    return rv;
-}
-
 void _ecp_conn_close(ECPConnection *conn) {
     if (_ecp_conn_is_open(conn)) {
         ecp_close_handler_t handler;
@@ -1618,6 +1582,7 @@ void ecp_err_handle(ECPConnection *conn, unsigned char mtype, int err) {
     rv = ecp_ext_err_handle(conn, mtype, err);
     if (rv != ECP_PASS) return;
 
+    if (err == ECP_ERR_CLOSED) return;
     if (ctx->handle_err) ctx->handle_err(conn, mtype, err);
 }
 
@@ -1643,12 +1608,52 @@ static ssize_t _send_ireq(ECPConnection *conn, ECPTimerItem *ti) {
 }
 
 static ssize_t _retry_ireq(ECPConnection *conn, ECPTimerItem *ti) {
+    ECPSocket *sock = conn->sock;
+    ECPDHKey key;
+    int is_open, refcount_ok;
     int rv;
 
-    rv = ecp_conn_reset_outb(conn);
+    rv = ecp_dhkey_gen(&key);
+    if (rv) return rv;
+
+#ifdef ECP_WITH_PTHREAD
+    pthread_mutex_lock(&sock->conn_table.mutex);
+    pthread_mutex_lock(&conn->mutex);
+#endif
+
+    rv = ECP_OK;
+    refcount_ok = 0;
+
+    is_open = _ecp_conn_is_open(conn);
+    if (!is_open) {
+        /* caller (timer) holds one reference */
+        refcount_ok = 1;
+        if (!ecp_conn_is_gc(conn)) refcount_ok++;
+
+        refcount_ok = (conn->refcount == refcount_ok);
+        if (refcount_ok) {
+            rv = conn_dhkey_new(conn, conn->key_curr, &key);
+        }
+    }
+
+#ifdef ECP_WITH_PTHREAD
+    pthread_mutex_unlock(&conn->mutex);
+    pthread_mutex_unlock(&sock->conn_table.mutex);
+#endif
     if (rv) return rv;
 
-    return _send_ireq(conn, ti);
+    if (is_open) {
+        /* already opened, that's OK */
+        return 0;
+    }
+
+    if (refcount_ok) {
+        return _send_ireq(conn, ti);
+    } else {
+        /* check later */
+        rv = ecp_timer_push(ti);
+        return rv;
+    }
 }
 
 ssize_t ecp_send_init_req(ECPConnection *conn, int retry) {
@@ -1911,7 +1916,7 @@ ssize_t ecp_send_open_rep(ECPConnection *conn) {
 
 ssize_t ecp_handle_open(ECPConnection *conn, unsigned char mtype, unsigned char *msg, size_t msg_size, ECP2Buffer *bufs) {
     ecp_open_handler_t handler;
-    int is_open, is_gc;
+    int is_reg, is_open, is_gc;
     int _rv = ECP_OK;
     ssize_t rv;
 
@@ -1919,37 +1924,55 @@ ssize_t ecp_handle_open(ECPConnection *conn, unsigned char mtype, unsigned char
     pthread_mutex_lock(&conn->mutex);
 #endif
 
+    is_reg = _ecp_conn_is_reg(conn);
     is_open = _ecp_conn_is_open(conn);
+    if (!is_reg) _rv = ECP_ERR_CLOSED;
+    if (!_rv && is_open) _rv = ECP_ERR;
+
+    /* set open if registered, avoids race condition with timeout handler */
+    if (!_rv) _ecp_conn_set_open(conn);
 
 #ifdef ECP_WITH_PTHREAD
     pthread_mutex_unlock(&conn->mutex);
 #endif
 
-    if (is_open) return ECP_ERR;
+    if (_rv) return _rv;
 
     if (mtype == ECP_MTYPE_OPEN_REQ) {
-        if (ecp_conn_is_outb(conn)) return ECP_ERR;
+        if (ecp_conn_is_outb(conn)) {
+            _rv = ECP_ERR;
+            goto handle_open_fin;
+        }
 
         rv = 2;
-        if (msg_size < rv) return ECP_ERR_SIZE;
+        if (msg_size < rv) {
+            _rv = ECP_ERR_SIZE;
+            goto handle_open_fin;
+        }
 
         if (msg[1]) rv += ECP_SIZE_VBOX;
-        if (msg_size < rv) return ECP_ERR_SIZE;
+        if (msg_size < rv) {
+            _rv = ECP_ERR_SIZE;
+            goto handle_open_fin;
+        }
     } else {
-        if (ecp_conn_is_inb(conn)) return ECP_ERR;
+        if (ecp_conn_is_inb(conn)) {
+            _rv = ECP_ERR;
+            goto handle_open_fin;
+        }
         rv = 0;
     }
 
     is_gc = ecp_conn_is_gc(conn);
     if (is_gc) {
         _rv = ecp_conn_insert_gc(conn);
-        if (_rv) return _rv;
+        if (_rv) goto handle_open_fin;
     }
 
     _rv = ecp_ext_conn_open(conn);
     if (_rv) {
         if (is_gc) ecp_conn_remove_gc(conn);
-        return _rv;
+        goto handle_open_fin;
     }
 
     handler = ecp_get_open_handler(conn);
@@ -1958,25 +1981,30 @@ ssize_t ecp_handle_open(ECPConnection *conn, unsigned char mtype, unsigned char
         if (_rv) {
             if (is_gc) ecp_conn_remove_gc(conn);
             ecp_ext_conn_close(conn);
-            return _rv;
+            goto handle_open_fin;
         }
     }
 
+    if (ecp_conn_is_inb(conn)) {
+        ecp_tr_release(bufs->packet, 1);
+        ecp_send_open_rep(conn);
+    } else if (ecp_conn_is_root(conn)) {
+        ecp_conn_remove_addr(conn);
+    }
+
+handle_open_fin:
+    if (_rv) {
 #ifdef ECP_WITH_PTHREAD
-    pthread_mutex_lock(&conn->mutex);
+        pthread_mutex_lock(&conn->mutex);
 #endif
 
-    _ecp_conn_set_open(conn);
+        _ecp_conn_clr_open(conn);
 
 #ifdef ECP_WITH_PTHREAD
-    pthread_mutex_unlock(&conn->mutex);
+        pthread_mutex_unlock(&conn->mutex);
 #endif
 
-    if (ecp_conn_is_inb(conn)) {
-        ecp_tr_release(bufs->packet, 1);
-        ecp_send_open_rep(conn);
-    } else if (ecp_conn_is_root(conn)) {
-        ecp_conn_remove_addr(conn);
+        return _rv;
     }
 
     return rv;
diff --git a/ecp/src/ecp/core.h b/ecp/src/ecp/core.h
index 30c00b3..6f1dbab 100644
--- a/ecp/src/ecp/core.h
+++ b/ecp/src/ecp/core.h
@@ -404,7 +404,6 @@ int ecp_cookie_verify(ECPSocket *sock, unsigned char *cookie, unsigned char *pub
 ECPConnection *ecp_conn_new_inb(ECPSocket *sock, unsigned char ctype);
 void ecp_conn_init(ECPConnection *conn, ECPSocket *sock, unsigned char ctype);
 int ecp_conn_init_outb(ECPConnection *conn);
-int ecp_conn_reset(ECPConnection *conn);
 void ecp_conn_set_flags(ECPConnection *conn, unsigned char flags);
 void ecp_conn_clr_flags(ECPConnection *conn, unsigned char flags);
 void ecp_conn_set_remote_key(ECPConnection *conn, ECPDHPub *key);
@@ -412,21 +411,19 @@ void ecp_conn_set_remote_addr(ECPConnection *conn, ecp_tr_addr_t *addr);
 int ecp_conn_create(ECPConnection *conn, ECPConnection *parent);
 int ecp_conn_create_inb(ECPConnection *conn, ECPConnection *parent, unsigned char s_idx, unsigned char c_idx, ecp_ecdh_public_t *public, ECPDHPub *rkey_perma, ecp_aead_key_t *shkey);
 int ecp_conn_create_outb(ECPConnection *conn, ECPConnection *parent, ECPNode *node);
-int ecp_conn_reset_outb(ECPConnection *conn);
 void ecp_conn_destroy(ECPConnection *conn);
 void ecp_conn_free(ECPConnection *conn);
 
 int ecp_conn_insert(ECPConnection *conn);
 int ecp_conn_insert_gc(ECPConnection *conn);
+int _ecp_conn_remove(ECPConnection *conn);
 void ecp_conn_remove(ECPConnection *conn);
 void ecp_conn_remove_addr(ECPConnection *conn);
 void ecp_conn_remove_gc(ECPConnection *conn);
 
 int _ecp_conn_open(ECPConnection *conn, ECPConnection *parent, ECPNode *node, int retry);
-int _ecp_conn_reopen(ECPConnection *conn, int retry);
 int ecp_conn_open(ECPConnection *conn, ECPNode *node);
 int ecp_conn_try_open(ECPConnection *conn, ECPNode *node);
-int ecp_conn_reopen(ECPConnection *conn);
 void _ecp_conn_close(ECPConnection *conn);
 void ecp_conn_close(ECPConnection *conn);
 
diff --git a/ecp/src/ecp/timer.c b/ecp/src/ecp/timer.c
index 6b58303..00478ed 100644
--- a/ecp/src/ecp/timer.c
+++ b/ecp/src/ecp/timer.c
@@ -181,14 +181,20 @@ ecp_sts_t ecp_timer_exe(ECPSocket *sock) {
                 _rv = retry(conn, to_exec+i);
                 if (_rv < 0) rv = _rv;
             }
-            if (rv && (rv != ECP_ERR_CLOSED)) {
+            if (rv) {
                 ecp_err_handle(conn, mtype, rv);
             }
         } else {
             rv = ECP_ERR_TIMEOUT;
-            ecp_err_handle(conn, mtype, rv);
             if (mtype == ECP_MTYPE_OPEN_REP) {
-                ecp_conn_remove(conn);
+                int _rv;
+
+                /* remove only if connection is not already open, avoids race condition with open reply message handler */
+                _rv = _ecp_conn_remove(conn);
+                if (_rv) rv = ECP_OK;
+            }
+            if (rv) {
+                ecp_err_handle(conn, mtype, rv);
             }
         }