cURL / Mailing Lists / curl-library / Single Mail

curl-library

[PATCH 6/7] Only assign NULL to data->easy_conn directly.

From: Carlo Wood <carlo_at_alinoe.com>
Date: Thu, 6 Nov 2014 03:03:20 +0100

This commits replaces occurances of *connp = NULL with
data->easy_conn = NULL, adding DEBUGASSERT to make sure
that this is allowed.

What this comes down to is that we only want to set easy_conn
to NULL when the connection is owned by the SessionHandle
that contains that easy_conn. Thus:

  data->easy_conn = NULL;

is only allowed while

  data->easy_conn->data == data

This is a preparation for the next commit that will need this.

Note that this commit also removes a duplicated variable
in curl_multi_remove_handle (nl. 'easy') because it was
an alias for 'data' and having two different variables for
the exact same thing did not make the code better understandable.
In particular, now we can read 'data->easy_conn->data == data'
and 'data->easy_conn->data = data' which matches the above ;)

Finally, in Curl_reconnect_request() we pass connp to
Curl_done() instead of &conn (where conn is a local variable).
The reason is that 'connp' fullfills the above assertion that
Curl_done needs while '&conn' doesn't. There is not functional
difference since 'conn' is not used below this call, *connp
is assigned NULL below that call, and initially conn == *connp.

---
 lib/multi.c    | 14 +++++++-------
 lib/transfer.c | 16 ++++++++++++----
 lib/url.c      | 32 ++++++++++++++++++++++++++++----
 3 files changed, 47 insertions(+), 15 deletions(-)
diff --git a/lib/multi.c b/lib/multi.c
index db1b582..2b5ec20 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -485,8 +485,7 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
                                    CURL *curl_handle)
 {
   struct Curl_multi *multi=(struct Curl_multi *)multi_handle;
-  struct SessionHandle *easy = curl_handle;
-  struct SessionHandle *data = easy;
+  struct SessionHandle *data = curl_handle;     /* easy handle */
   bool premature;
   bool easy_owns_conn;
   struct curl_llist_element *e;
@@ -505,7 +504,7 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
 
 
   premature = (data->mstate < CURLM_STATE_COMPLETED) ? TRUE : FALSE;
-  easy_owns_conn = (data->easy_conn && (data->easy_conn->data == easy)) ?
+  easy_owns_conn = (data->easy_conn && (data->easy_conn->data == data)) ?
     TRUE : FALSE;
 
   /* If the 'state' is not INIT or COMPLETED, we might need to do something
@@ -526,7 +525,7 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
     connclose(data->easy_conn, "Removed with partial response");
     /* Set connection owner so that Curl_done() closes it.
        We can safely do this here since connection is killed. */
-    data->easy_conn->data = easy;
+    data->easy_conn->data = data;
   }
 
   /* The timer must be shut down before data->multi is set to NULL,
@@ -573,7 +572,7 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
   /* change state without using multistate(), only to make singlesocket() do
      what we want */
   data->mstate = CURLM_STATE_COMPLETED;
-  singlesocket(multi, easy); /* to let the application know what sockets that
+  singlesocket(multi, data); /* to let the application know what sockets that
                                 vanish with this handle */
 
   /* Remove the association between the connection and the handle */
@@ -590,7 +589,7 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
   for(e = multi->msglist->head; e; e = e->next) {
     struct Curl_message *msg = e->ptr;
 
-    if(msg->extmsg.easy_handle == easy) {
+    if(msg->extmsg.easy_handle == data) {
       Curl_llist_remove(multi->msglist, e, NULL);
       /* there can only be one from this specific handle */
       break;
@@ -971,7 +970,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
     if(data->easy_conn && data->mstate > CURLM_STATE_CONNECT &&
        data->mstate < CURLM_STATE_COMPLETED)
-      /* Make sure we set the connection's current owner */
+      /* Make sure we set the connection owner to the current handle,
+         a lot of code called from here depends on that. */
       data->easy_conn->data = data;
 
     if(data->easy_conn &&
diff --git a/lib/transfer.c b/lib/transfer.c
index b48dfce..3def3fa 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -1824,12 +1824,20 @@ Curl_reconnect_request(struct connectdata **connp)
 
   infof(data, "Re-used connection seems dead, get a new one\n");
 
+  /* This is true because we only get here from Curl_do which is only
+   * called from multi_runsingle in state CURLM_STATE_DO, and for
+   * that state sets: data->easy_conn->data = data before calling
+   * Curl_do with &data->easy_conn as 'connp' argument, hence: */
+  DEBUGASSERT(connp == &data->easy_conn);
+
   connclose(conn, "Reconnect dead connection"); /* enforce close */
-  result = Curl_done(&conn, result, FALSE); /* we are so done with this */
+  result = Curl_done(connp, result, FALSE); /* we are so done with this */
 
-  /* conn may no longer be a good pointer, clear it to avoid mistakes by
-     parent functions */
-  *connp = NULL;
+  /* *connp may no longer be a good pointer, clear it to avoid mistakes by
+     parent functions -- note that Curl_done above might already have
+     done this for us. */
+  if(data->easy_conn)
+    data->easy_conn = NULL;
 
   /*
    * According to bug report #1330310. We need to check for CURLE_SEND_ERROR
diff --git a/lib/url.c b/lib/url.c
index b41f5cb..9fa7228 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -5228,6 +5228,9 @@ static CURLcode create_conn(struct SessionHandle *data,
     goto out;
   }
 
+  /* Overwriting a non-NULL value would be bad */
+  DEBUGASSERT(*in_connect == NULL);
+
   /* We must set the return variable as soon as possible, so that our
      parent can cleanup any possible allocs we may have done before
      any failure */
@@ -5626,6 +5629,9 @@ static CURLcode create_conn(struct SessionHandle *data,
       infof(data, "No connections available.\n");
 
       conn_free(conn);
+      /* It's ok to set *in_connect to NULL here because it was initially NULL
+         when we entered this function and therefore cannot be shared by
+         multiple easy handles in a pipeline. */
       *in_connect = NULL;
 
       result = CURLE_NO_CONNECTION_AVAILABLE;
@@ -5790,6 +5796,10 @@ CURLcode Curl_connect(struct SessionHandle *data,
 
   *asyncp = FALSE; /* assume synchronous resolves by default */
 
+  /* This is required by create_conn() as well as so that we can reset
+     the value on error without worries, if anything goes wrong */
+  DEBUGASSERT(*in_connect == NULL);
+
   /* call the stuff that needs to be called */
   result = create_conn(data, in_connect, asyncp);
 
@@ -5806,6 +5816,10 @@ CURLcode Curl_connect(struct SessionHandle *data,
     }
   }
 
+  /* It's ok to set *in_connect to NULL here because it was initially NULL
+     when we entered this function and therefore cannot be shared by multiple
+     easy handles in a pipeline. */
+
   if(result == CURLE_NO_CONNECTION_AVAILABLE) {
     *in_connect = NULL;
     return result;
@@ -5938,10 +5952,20 @@ CURLcode Curl_done(struct connectdata **connp,
       data->state.lastconnect = NULL;
   }
 
-  *connp = NULL; /* to make the caller of this function better detect that
-                    this was either closed or handed over to the connection
-                    cache here, and therefore cannot be used from this point on
-                 */
+  /* This is true because we only get here from Curl_reconnect_request
+   * where the exact same assert holds, or from curl_multi_remove_handle
+   * which only calls Curl_done when data->easy_conn->data == data, or from
+   * multi_runsingle with various states (PROTOCONNECT, DO, DOING, DO_MORE,
+   * PERFORM and DONE), all of which set: data->easy_conn->data = data
+   * before calling Curl_done with &data->easy_conn as 'connp' argument,
+   * hence:
+   */
+  DEBUGASSERT(connp == &data->easy_conn);
+
+  data->easy_conn = NULL; /* to make the caller of this function better detect
+                             that this was either closed or handed over to the
+                             connection cache here, and therefore cannot be
+                             used from this point on */
   Curl_free_request_state(data);
 
   return result;
-- 
2.1.1
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html
Received on 2014-11-06