Try to fix problem if balancing free client DCB and free session so that auth data is always available and client DCB is not freed until session is ready to be freed. Also fix problem in auth logic.

server/core/dcb.c

@@ -126,8 +126,6 @@ static int gw_write(DCB *dcb, bool *stop_writing);
 static int gw_write_SSL(DCB *dcb, bool *stop_writing);
 static void dcb_log_errors_SSL (DCB *dcb, const char *called_by, int ret);
 
-static void mysql_auth_free_client_data(DCB *dcb);
-
 size_t dcb_get_session_id(
     DCB *dcb)
 {
@@ -320,8 +318,6 @@ dcb_clone(DCB *orig)
 static void
 dcb_final_free(DCB *dcb)
 {
-    DCB_CALLBACK *cb;
-
     CHK_DCB(dcb);
     ss_info_dassert(dcb->state == DCB_STATE_DISCONNECTED ||
                     dcb->state == DCB_STATE_ALLOC,
@@ -384,10 +380,24 @@ dcb_final_free(DCB *dcb)
         if (SESSION_STATE_DUMMY != local_session->state)
         {
             session_free(local_session);
+            return;
         }
     }
+    dcb_free_all_memory(dcb);
+}
 
-    mysql_auth_free_client_data(dcb);
+/**
+ * Free the memory belonging to a DCB
+ *
+ * NB The DCB is fully detached from all links except perhaps the session
+ * dcb_client link.
+ *
+ * @param dcb The DCB to free
+ */
+void
+dcb_free_all_memory(DCB *dcb)
+{
+    DCB_CALLBACK *cb_dcb;
 
     if (dcb->protocol && (!DCB_IS_CLONE(dcb)))
     {
@@ -424,10 +434,10 @@ dcb_final_free(DCB *dcb)
     }
 
     spinlock_acquire(&dcb->cb_lock);
-    while ((cb = dcb->callbacks) != NULL)
+    while ((cb_dcb = dcb->callbacks) != NULL)
     {
-        dcb->callbacks = cb->next;
-        free(cb);
+        dcb->callbacks = cb_dcb->next;
+        free(cb_dcb);
     }
     spinlock_release(&dcb->cb_lock);
     if (dcb->ssl)
@@ -3025,20 +3035,3 @@ dcb_role_name(DCB *dcb)
     return name;
 }
 
-/**
- * @brief Free the client data pointed to by the passed DCB.
- *
- * Currently all that is required is to free the storage pointed to by
- * dcb->data.  But this is intended to be implemented as part of the
- * authentication API at which time this code will be moved into the
- * MySQL authenticator.  If the data structure were to become more complex
- * the mechanism would still work and be the responsibility of the authenticator.
- * The DCB should not know authenticator implementation details.
- *
- * @param dcb Request handler DCB connected to the client
- */
-static void
-mysql_auth_free_client_data(DCB *dcb)
-{
-    free(dcb->data);
-}

server/core/session.c

@@ -65,6 +65,8 @@ static SPINLOCK timeout_lock = SPINLOCK_INIT;
 static int session_setup_filters(SESSION *session);
 static void session_simple_free(SESSION *session, DCB *dcb);
 
+static void mysql_auth_free_client_data(DCB *dcb);
+
 /**
  * Allocate a new session for a new client of the specified service.
  *
@@ -91,23 +93,6 @@ session_alloc(SERVICE *service, DCB *client_dcb)
                   "session object due error %d, %s.",
                   errno,
                   strerror_r(errno, errbuf, sizeof(errbuf)));
-        /* Does this possibly need a lock? */
-        /*
-         * This is really not the right way to do this.  The data in a DCB is
-         * router specific and should be freed by a function in the relevant
-         * router.  This would be better achieved by placing a function reference
-         * in the DCB and having dcb_final_free call it to dispose of the data
-         * at the final destruction of the DCB.  However, this piece of code is
-         * only run following a calloc failure, so the system is probably on
-         * the point of crashing anyway.
-         *
-         */
-        if (client_dcb->data && !DCB_IS_CLONE(client_dcb))
-        {
-            void * clientdata = client_dcb->data;
-            client_dcb->data = NULL;
-            free(clientdata);
-        }
         return NULL;
     }
 #if defined(SS_DEBUG)
@@ -437,6 +422,14 @@ session_free(SESSION *session)
     spinlock_release(&session_spin);
     atomic_add(&session->service->stats.n_current, -1);
 
+    /***
+     *
+     */
+    if (session->client_dcb)
+    {
+        mysql_auth_free_client_data(session->client_dcb);
+        dcb_free_all_memory(session->client_dcb);
+    }
     /**
      * If session is not child of some other session, free router_session.
      * Otherwise let the parent free it.
@@ -1108,3 +1101,21 @@ sessionGetList(SESSIONLISTFILTER filter)
 
     return set;
 }
+
+/**
+ * @brief Free the client data pointed to by the passed DCB.
+ *
+ * Currently all that is required is to free the storage pointed to by
+ * dcb->data.  But this is intended to be implemented as part of the
+ * authentication API at which time this code will be moved into the
+ * MySQL authenticator.  If the data structure were to become more complex
+ * the mechanism would still work and be the responsibility of the authenticator.
+ * The DCB should not know authenticator implementation details.
+ *
+ * @param dcb Request handler DCB connected to the client
+ */
+static void
+mysql_auth_free_client_data(DCB *dcb)
+{
+    free(dcb->data);
+}

server/include/dcb.h

@@ -294,6 +294,7 @@ DCB *dcb_get_zombies(void);
 int dcb_write(DCB *, GWBUF *);
 DCB *dcb_alloc(dcb_role_t);
 void dcb_free(DCB *);
+void dcb_free_all_memory(DCB *dcb);
 DCB *dcb_connect(struct server *, struct session *, const char *);
 DCB *dcb_clone(DCB *);
 int dcb_read(DCB *, GWBUF **, int);

server/modules/protocol/mysql_backend.c

@@ -172,7 +172,7 @@ static int gw_read_backend_event(DCB *dcb)
         goto return_rc;
     }
 
-    if (dcb->session == NULL)
+    if (dcb->dcb_is_zombie || dcb->session == NULL)
     {
         goto return_rc;
     }

server/modules/protocol/mysql_client.c

@@ -537,7 +537,7 @@ int gw_read_client_event(DCB* dcb)
                     auth_val = MYSQL_AUTH_NO_SESSION;
                 }
             }
-            if (MYSQL_AUTH_SUCCEEDED != auth_val)
+            if (MYSQL_AUTH_SUCCEEDED != auth_val && MYSQL_AUTH_SSL_INCOMPLETE != auth_val)
             {
                 protocol->protocol_auth_state = MYSQL_AUTH_FAILED;
                 mysql_client_auth_error_handling(dcb, auth_val);