On 1/7/07, Daniel Stenberg <daniel_at_haxx.se> wrote:

> It is a rather complex situation

Man, you got that right :-)

As far as I can tell, here is what's happening...

Handle ONE grabs connection CONN and when it reaches CURLM_STATE_DONE,
it calls Curl_done() which sets the CONN->inuse flag to FALSE.

Then handle TWO comes along, and since CONN->inuse is FALSE it grabs CONN,
and sets the inuse flag to TRUE.

Now curl_multi_remove_handle() gets called on handle ONE, which calls Curl_done
again and sets the inuse flag back to FALSE, even though handle TWO is actually
using the connection.

Then handle THREE comes along, and grabs the CONN even though handle TWO is
still using it, because curl_multi_remove_handle(ONE) made it look available.

If handle TWO's transfer fails, it can't be sure if CONN is still reliable,
so it does conn_free(CONN), which leaves handle THREE holding a dead pointer.

I'm not sure what the right solution is, but the comment/code around line
548 of curl_multi_remove_handle() doesn't look right to me:

    if(easy->easy_conn) {
      /* Set up the association right */
      easy->easy_conn->data = easy->easy_handle;

      /* Curl_done() clears the conn->data field to lose the association
         between the easy handle and the connection */
      Curl_done(&easy->easy_conn, easy->result, premature);

      if(easy->easy_conn)
        /* the connection is still alive, set back the association to enable
           the check below to trigger TRUE */
        easy->easy_conn->data = easy->easy_handle;
    }

I don't understand the comment about "Set up the association right"

If the easy->easy_conn->data points to a different handle than the
easy->easy_handle does, that would indicate to me that some other
handle is already using the connection, and calling Curl_done()
here is quite likely to crash something on down the line.

It seems like the code should *check* the conn<-->handle association,
instead of *change* it, maybe something like this:

  if(easy->easy_conn && (easy->easy_conn->data == easy->easy_handle)) {

      /* Curl_done() clears the conn->data field to lose the association
         between the easy handle and the connection */
      Curl_done(&easy->easy_conn, easy->result, premature);

      if(easy->easy_conn)
        /* the connection is still alive, set back the association to enable
           the check below to trigger TRUE */
        easy->easy_conn->data = easy->easy_handle;
    }

I don't know why Curl_done() gets called twice for the same handle anyway,
maybe multi_remove_handle() only needs it when state<STATE_COMPLETED?

Does any of this make sense?

 - Jeff
Received on 2007-02-15