[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1176: DISPATCH-2046: prevent crash when accessing connector pointer

[GitBox <gi...@apache.org>]

kgiusti commented on a change in pull request #1176:
URL: https://github.com/apache/qpid-dispatch/pull/1176#discussion_r623201753



##########
File path: src/server.c
##########
@@ -1644,25 +1657,31 @@ void qd_listener_decref(qd_listener_t* li)
 
 qd_connector_t *qd_server_connector(qd_server_t *server)
 {
-    qd_connector_t *ct = new_qd_connector_t();
-    if (!ct) return 0;
-    ZERO(ct);
-    sys_atomic_init(&ct->ref_count, 1);
-    ct->server  = server;
-    qd_failover_item_list_t conn_info_list;
-    DEQ_INIT(conn_info_list);
-    ct->conn_info_list = conn_info_list;
-    ct->conn_index = 1;
-    ct->state   = CXTR_STATE_INIT;
-    ct->lock = sys_mutex();
-    ct->conn_msg = (char*) malloc(300);
-    memset(ct->conn_msg, 0, 300);
-    ct->timer = qd_timer(ct->server->qd, try_open_cb, ct);
-    if (!ct->lock || !ct->timer) {
-        qd_connector_decref(ct);
-        return 0;
-    }
-    return ct;
+    qd_connector_t *connector = new_qd_connector_t();
+    if (!connector) return 0;
+    ZERO(connector);
+    sys_atomic_init(&connector->ref_count, 1);
+    DEQ_INIT(connector->conn_info_list);
+
+    connector->server  = server;

Review comment:
       How does that make a difference?  I don't understand - the other stuff (conn_index, server, etc) are idempotent - it doesn't matter if they are set or not if the initialization fails.
   
   If it's a matter of preference then no problem - I just don't see any advantage to that change.

##########
File path: src/server.c
##########
@@ -1644,25 +1657,31 @@ void qd_listener_decref(qd_listener_t* li)
 
 qd_connector_t *qd_server_connector(qd_server_t *server)
 {
-    qd_connector_t *ct = new_qd_connector_t();
-    if (!ct) return 0;
-    ZERO(ct);
-    sys_atomic_init(&ct->ref_count, 1);
-    ct->server  = server;
-    qd_failover_item_list_t conn_info_list;
-    DEQ_INIT(conn_info_list);
-    ct->conn_info_list = conn_info_list;
-    ct->conn_index = 1;
-    ct->state   = CXTR_STATE_INIT;
-    ct->lock = sys_mutex();
-    ct->conn_msg = (char*) malloc(300);
-    memset(ct->conn_msg, 0, 300);
-    ct->timer = qd_timer(ct->server->qd, try_open_cb, ct);
-    if (!ct->lock || !ct->timer) {
-        qd_connector_decref(ct);
-        return 0;
-    }
-    return ct;
+    qd_connector_t *connector = new_qd_connector_t();
+    if (!connector) return 0;
+    ZERO(connector);
+    sys_atomic_init(&connector->ref_count, 1);
+    DEQ_INIT(connector->conn_info_list);
+
+    connector->server  = server;
+    connector->conn_index = 1;
+    connector->state = CXTR_STATE_INIT;
+    connector->lock = sys_mutex();
+    if (!connector->lock)
+        goto error;
+    connector->timer = qd_timer(connector->server->qd, try_open_cb, connector);
+    if (!connector->timer)
+        goto error;
+    connector->conn_msg = (char*) malloc(300);
+    if (!connector->conn_msg)
+        goto error;
+    memset(connector->conn_msg, 0, 300);
+    return connector;
+
+error:

Review comment:
       Triggered!
   
   goto's for error handling in C code are not evil.  In fact, when using an RAII resource allocation model they simplify the code.  For example, if I had to check every allocation and clean up on each check, you'd get:
   
   `allocate_lock();`
   `if (connector->lock) {`
   `   allocate_timer();`
   `   if (timer) {`
   `      allocate conn_msg;`
   `      if (conn_msg) {`
   `          memset conn_msg;`
   `          return connector`
   `      } else {`
   `           free_timer();`
   `           free_lock();`
   `   } else {`
   `      free_lock();`
   `   }`
   `}`
   `decref();`
   `return 0;`
   
   
   Is that code actually better than the goto version?    Now assume you make a change where you need to allocate something else.  In the if case you'll have to add another level of indentation and update each separate cleanup "else" to deal with unwinding the new resource.
   
   Google "arrow anti-pattern" for another example.

##########
File path: src/connection_manager.c
##########
@@ -790,6 +790,8 @@ qd_error_t qd_entity_refresh_connector(qd_entity_t* entity, void *impl)
     int i = 1;
     int num_items = 0;
 
+    sys_mutex_lock(ct->lock);
+

Review comment:
       Remember the connector will be in the "deleted" state until the related connection has closed. 
   Would it be better to report the state as "CLOSING" instead of "DELETED"?  At least if - for whatever reason - the connection hasn't closed we don't ignore connectors that still exist.

##########
File path: src/server.c
##########
@@ -1155,56 +1156,55 @@ static qd_failover_item_t *qd_connector_get_conn_info(qd_connector_t *ct) {
 
 
 /* Timer callback to try/retry connection open */
-static void try_open_lh(qd_connector_t *ct)
+static void try_open_lh(qd_connector_t *connector)
 {
-    if (ct->state != CXTR_STATE_CONNECTING && ct->state != CXTR_STATE_INIT) {
-        /* No longer referenced by pn_connection or timer */
-        qd_connector_decref(ct);
+    // Allocate a new connection. No other thread will touch this
+    // connection until pn_proactor_connect is called below
+    qd_connection_t *qd_conn = qd_server_connection(connector->server, &connector->config);
+    if (!qd_conn) {                 /* Try again later */
+        qd_log(connector->server->log_source, QD_LOG_CRITICAL, "Allocation failure connecting to %s",
+               connector->config.host_port);
+        connector->delay = 10000;
+        connector->state = CXTR_STATE_CONNECTING;

Review comment:
       It needs to be CONNECTING.  The CONNECTING state indicates the timer has been scheduled (and shouldn't be rescheduled).
   FAILED means the connection has dropped and we're waiting for the connection to be cleaned up before we schedule the timer.
   
   They are two different states - this must be CONNECTING.

##########
File path: src/connection_manager.c
##########
@@ -859,16 +861,23 @@ qd_error_t qd_entity_refresh_connector(qd_entity_t* entity, void *impl)
     case CXTR_STATE_INIT:
       state_info = "INITIALIZING";
       break;
+    case CXTR_STATE_DELETED:
+      state_info = "DELETED";
+      break;
     default:
       state_info = "UNKNOWN";
       break;
     }
 
     if (qd_entity_set_string(entity, "failoverUrls", failover_info) == 0
-            && qd_entity_set_string(entity, "connectionStatus", state_info) == 0
-            && qd_entity_set_string(entity, "connectionMsg", ct->conn_msg) == 0)
+        && qd_entity_set_string(entity, "connectionStatus", state_info) == 0
+        && qd_entity_set_string(entity, "connectionMsg", ct->conn_msg) == 0) {
+
+        sys_mutex_unlock(ct->lock);

Review comment:
       Yes, I agree.  Many changes in this patch is to clarify the code by replacing variable names like "ct" and "ctx", which are used inconsistently to sometimes mean "connector" or sometimes mean "qd_connection" with actual unambiguous names such as "connector" or "qd_conn".
   
   While short names make typing easier, it makes reading the code harder.  An we end up reading the code more often than writing it.

##########
File path: src/alloc_pool.c
##########
@@ -90,7 +90,7 @@ DEQ_DECLARE(qd_alloc_type_t, qd_alloc_type_list_t);
 #if QD_MEMORY_STATS
 static const char *leaking_types[] = {
     // DISPATCH-1679:
-    "qd_timer_t", "qd_connector_t",
+    //"qd_timer_t", "qd_connector_t",

Review comment:
       Yes that needs to be removed - will do




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org