Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

invalid `conn->data` pointer in ConnectionExists when multiplexing #4845

Closed
dmitrmax opened this issue Jan 23, 2020 · 34 comments
Closed

invalid `conn->data` pointer in ConnectionExists when multiplexing #4845

dmitrmax opened this issue Jan 23, 2020 · 34 comments
Labels

Comments

@dmitrmax
Copy link

@dmitrmax dmitrmax commented Jan 23, 2020

I did this

Actually I'm trying to reopen issue #4062 as I have new details.

valgrind run app with libcurl.so with nghttp2 1.38.0

I expected the following

no detected problems with memory access

curl/libcurl version

7.66.0

==307== Invalid read of size 8
==307== at 0x603CBC1: ConnectionExists (url.c:1190)
==307== by 0x6041FD6: create_conn (url.c:3704)
==307== by 0x60427C3: Curl_connect (url.c:3972)
==307== by 0x6008A42: multi_runsingle (multi.c:1374)
==307== by 0x600AFD1: multi_socket (multi.c:2553)
==307== by 0x600B6A2: curl_multi_socket_action (multi.c:2666)

1189 if(CONN_INUSE(check) && check->data &&
1190 (check->data->multi != needle->data->multi))

As far as I tracked this problem it happens only on HTTP/2.

Invalid read is about this access: check->data->multi

It happens right after easy handle with value in check->data is removed from multi with curl_multi_remove_handle() call.

Somehow easy handle is left in the connection cache. Actually I don't see the code, which is responsible for connection cache cleaning during curl_multi_remove_handle() and CONN_INUSE(check) is 1 for this connection.

Probably there is a case when detach_connection() is not called for this easy handle in the multi.c file.

operating system

Linux (CentOS 7.7)

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 23, 2020

Additionally I can provide following details:

  1. It happens when there are multiple requests on the way.
  2. It happens randomly, i.e. not after first or any specific number of requests.
  3. It happens only once during whole program run. You can even create multiple independent multi handles, but valgrind shows this invalid access just once.
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 23, 2020

This looks like a problem we've already fixed. Can you reproduce this with 7.68.0?

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 23, 2020

@bagder, thanks for quick feedback. I'm gonna try this now. I've looked throughout changelog before posting the issue but haven't found any

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 23, 2020

@bagder , tried with 7.68.0 - I still can reproduce the problem

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 23, 2020

I've checked that this condition inside curl_multi_remove_handle() is not met
/* Remove the association between the connection and the handle */
if(data->conn) {
data->conn->data = NULL;
detach_connnection(data);
}

So detach_connection() is really not called. This is why CONN_INUSE returns true - actual value of c->easyq.size is 1.

So, it seems that in HTTP/2 there is a code path when data->conn is NULL, while data actually is backed up by some connection. Sadly I'm not into connection management of curl :(

Upd: seems that this is wrong path of investigation. detach_connection() is happened for this connection before.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 23, 2020

It might be worth trying to create a stand-alone version of the code that can reproduce this problem so that you can share that with us and we can help out with debugging from our ends...

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 23, 2020

@bagder , this seems too complicated, because it depends on a pretty big piece of code that combines curl with boost::asio.

Anyway, seems that I've found the problem. This code is at the end of ConnectionExists function in url.c:

  if(chosen) {
    /* mark it as used before releasing the lock */
    chosen->data = data; /* own it! */
    Curl_conncache_unlock(data);
    *usethis = chosen;
    return TRUE; /* yes, we found one to use! */
  }

Here the easy handle data is remembered in the connection chosen. After the easy handle is removed from multi handle nobody checks if any connectdata object belonging to the bundle still refers to the removed easy handle. Later on this removed easy handle is dereferenced as check->data->multi

I only do not understand why valgrind finds such case strictly once per program run though it seems that this situation have to happen again and again.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 23, 2020

Are you perchance using a shared connection cache?

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 23, 2020

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 23, 2020

It's not meaningful if you only use a single multi handle in a single thread, no, since a multi handle already shares the connection cache with all the added easy handles.

I asked because a shared connection cache makes a different set of race conditions (when two threads share the same cache) that I thought could be playing a part here. But then I was wrong.

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 23, 2020

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 24, 2020

You need to give me a lot more details if you want my help as this is really hard for me to debug without a recipe that reproduces in my end.

The problem happens when you add a new transfer. I presume the previous transfer(s) still going at that point are intended? Are they many? Did any previous transfer fail (early) ? Can you add a debug output that counts users/transfers on each connection as an aid to see if we can detect when that gets wrong at first.

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 24, 2020

Ok, Daniel. I've taken your example https://curl.haxx.se/libcurl/c/asiohiper.html and adapted it to make use of HTTP/2 and multiple outstanding request. Valgrind reports invalid read in event_cb - ignore it (it's a bug in the example, btw). Just a couple of seconds run and you'll get invalid read inside the ConnectionExists function inside curl.

Hope it will help. Here is the code:

#include <curl/curl.h>
#include <boost/asio.hpp>
#include <boost/bind.hpp>
#include <iostream>
#include <map>
#include <chrono>

#define MSG_OUT stdout

/* boost::asio related objects
 * using global variables for simplicity
 */
boost::asio::io_service io_service;
boost::asio::steady_timer timer(io_service);
std::map<curl_socket_t, boost::asio::ip::tcp::socket *> socket_map;

/* Global information, common to all connections */
typedef struct _GlobalInfo
{
  CURLM *multi;
  int still_running;
  int on_the_way;
} GlobalInfo;

/* Information associated with a specific easy handle */
typedef struct _ConnInfo
{
  CURL *easy;
  char *url;
  GlobalInfo *global;
  char error[CURL_ERROR_SIZE];
} ConnInfo;

static void timer_cb(const boost::system::error_code & error, GlobalInfo *g);

/* Update the event timer after curl_multi library calls */
static int multi_timer_cb(CURLM *multi, long timeout_ms, GlobalInfo *g)
{
  fprintf(MSG_OUT, "\nmulti_timer_cb: timeout_ms %ld", timeout_ms);

  /* cancel running timer */
  timer.cancel();

  if(timeout_ms > 0) {
    /* update timer */
    timer.expires_from_now(std::chrono::milliseconds(timeout_ms));
    timer.async_wait(boost::bind(&timer_cb, _1, g));
  }
  else if(timeout_ms == 0) {
    /* call timeout function immediately */
    boost::system::error_code error; /*success*/
    timer_cb(error, g);
  }

  return 0;
}

/* Die if we get a bad CURLMcode somewhere */
static void mcode_or_die(const char *where, CURLMcode code)
{
  if(CURLM_OK != code) {
    const char *s;
    switch(code) {
    case CURLM_CALL_MULTI_PERFORM:
      s = "CURLM_CALL_MULTI_PERFORM";
      break;
    case CURLM_BAD_HANDLE:
      s = "CURLM_BAD_HANDLE";
      break;
    case CURLM_BAD_EASY_HANDLE:
      s = "CURLM_BAD_EASY_HANDLE";
      break;
    case CURLM_OUT_OF_MEMORY:
      s = "CURLM_OUT_OF_MEMORY";
      break;
    case CURLM_INTERNAL_ERROR:
      s = "CURLM_INTERNAL_ERROR";
      break;
    case CURLM_UNKNOWN_OPTION:
      s = "CURLM_UNKNOWN_OPTION";
      break;
    case CURLM_LAST:
      s = "CURLM_LAST";
      break;
    default:
      s = "CURLM_unknown";
      break;
    case CURLM_BAD_SOCKET:
      s = "CURLM_BAD_SOCKET";
      fprintf(MSG_OUT, "\nERROR: %s returns %s", where, s);
      /* ignore this error */
      return;
    }

    fprintf(MSG_OUT, "\nERROR: %s returns %s", where, s);

    exit(code);
  }
}

/* Check for completed transfers, and remove their easy handles */
static void check_multi_info(GlobalInfo *g)
{
  char *eff_url;
  CURLMsg *msg;
  int msgs_left;
  ConnInfo *conn;
  CURL *easy;
  CURLcode res;

  fprintf(MSG_OUT, "\nREMAINING: %d", g->still_running);

  while((msg = curl_multi_info_read(g->multi, &msgs_left))) {
    if(msg->msg == CURLMSG_DONE) {
      easy = msg->easy_handle;
      res = msg->data.result;
      curl_easy_getinfo(easy, CURLINFO_PRIVATE, &conn);
      curl_easy_getinfo(easy, CURLINFO_EFFECTIVE_URL, &eff_url);
      fprintf(MSG_OUT, "\nDONE: %s => (%d) %s", eff_url, res, conn->error);
      curl_multi_remove_handle(g->multi, easy);
      free(conn->url);
      curl_easy_cleanup(easy);
      free(conn);
      g->on_the_way--;
    }
  }
}

/* Called by asio when there is an action on a socket */
static void event_cb(GlobalInfo *g, curl_socket_t s,
                     int action, const boost::system::error_code & error,
                     int *fdp)
{
  fprintf(MSG_OUT, "\nevent_cb: action=%d", action);

  if(socket_map.find(s) == socket_map.end()) {
    fprintf(MSG_OUT, "\nevent_cb: socket already closed");
    return;
  }

  /* make sure the event matches what are wanted */
  if(*fdp == action || *fdp == CURL_POLL_INOUT) {
    CURLMcode rc;
    if(error)
      action = CURL_CSELECT_ERR;
    rc = curl_multi_socket_action(g->multi, s, action, &g->still_running);

    mcode_or_die("event_cb: curl_multi_socket_action", rc);
    check_multi_info(g);

    if(g->still_running <= 0) {
      fprintf(MSG_OUT, "\nlast transfer done, kill timeout");
      timer.cancel();
    }

    /* keep on watching.
     * the socket may have been closed and/or fdp may have been changed
     * in curl_multi_socket_action(), so check them both */
    if(!error && socket_map.find(s) != socket_map.end() &&
       (*fdp == action || *fdp == CURL_POLL_INOUT)) {
      boost::asio::ip::tcp::socket *tcp_socket = socket_map.find(s)->second;

      if(action == CURL_POLL_IN) {
        tcp_socket->async_read_some(boost::asio::null_buffers(),
                                    boost::bind(&event_cb, g, s,
                                                action, _1, fdp));
      }
      if(action == CURL_POLL_OUT) {
        tcp_socket->async_write_some(boost::asio::null_buffers(),
                                     boost::bind(&event_cb, g, s,
                                                 action, _1, fdp));
      }
    }
  }
}

/* Called by asio when our timeout expires */
static void timer_cb(const boost::system::error_code & error, GlobalInfo *g)
{
  if(!error) {
    fprintf(MSG_OUT, "\ntimer_cb: ");

    CURLMcode rc;
    rc = curl_multi_socket_action(g->multi, CURL_SOCKET_TIMEOUT, 0,
                                  &g->still_running);

    mcode_or_die("timer_cb: curl_multi_socket_action", rc);
    check_multi_info(g);
  }
}

/* Clean up any data */
static void remsock(int *f, GlobalInfo *g)
{
  fprintf(MSG_OUT, "\nremsock: ");

  if(f) {
    free(f);
  }
}

static void setsock(int *fdp, curl_socket_t s, CURL *e, int act, int oldact,
                    GlobalInfo *g)
{
  fprintf(MSG_OUT, "\nsetsock: socket=%d, act=%d, fdp=%p", s, act, fdp);

  std::map<curl_socket_t, boost::asio::ip::tcp::socket *>::iterator it =
    socket_map.find(s);

  if(it == socket_map.end()) {
    fprintf(MSG_OUT, "\nsocket %d is a c-ares socket, ignoring", s);
    return;
  }

  boost::asio::ip::tcp::socket * tcp_socket = it->second;

  *fdp = act;

  if(act == CURL_POLL_IN) {
    fprintf(MSG_OUT, "\nwatching for socket to become readable");
    if(oldact != CURL_POLL_IN && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_read_some(boost::asio::null_buffers(),
                                  boost::bind(&event_cb, g, s,
                                              CURL_POLL_IN, _1, fdp));
    }
  }
  else if(act == CURL_POLL_OUT) {
    fprintf(MSG_OUT, "\nwatching for socket to become writable");
    if(oldact != CURL_POLL_OUT && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_write_some(boost::asio::null_buffers(),
                                   boost::bind(&event_cb, g, s,
                                               CURL_POLL_OUT, _1, fdp));
    }
  }
  else if(act == CURL_POLL_INOUT) {
    fprintf(MSG_OUT, "\nwatching for socket to become readable & writable");
    if(oldact != CURL_POLL_IN && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_read_some(boost::asio::null_buffers(),
                                  boost::bind(&event_cb, g, s,
                                              CURL_POLL_IN, _1, fdp));
    }
    if(oldact != CURL_POLL_OUT && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_write_some(boost::asio::null_buffers(),
                                   boost::bind(&event_cb, g, s,
                                               CURL_POLL_OUT, _1, fdp));
    }
  }
}

static void addsock(curl_socket_t s, CURL *easy, int action, GlobalInfo *g)
{
  /* fdp is used to store current action */
  int *fdp = (int *) calloc(sizeof(int), 1);

  setsock(fdp, s, easy, action, 0, g);
  curl_multi_assign(g->multi, s, fdp);
}

/* CURLMOPT_SOCKETFUNCTION */
static int sock_cb(CURL *e, curl_socket_t s, int what, void *cbp, void *sockp)
{
  fprintf(MSG_OUT, "\nsock_cb: socket=%d, what=%d, sockp=%p", s, what, sockp);

  GlobalInfo *g = (GlobalInfo*) cbp;
  int *actionp = (int *) sockp;
  const char *whatstr[] = { "none", "IN", "OUT", "INOUT", "REMOVE"};

  fprintf(MSG_OUT,
          "\nsocket callback: s=%d e=%p what=%s ", s, e, whatstr[what]);

  if(what == CURL_POLL_REMOVE) {
    fprintf(MSG_OUT, "\n");
    remsock(actionp, g);
  }
  else {
    if(!actionp) {
      fprintf(MSG_OUT, "\nAdding data: %s", whatstr[what]);
      addsock(s, e, what, g);
    }
    else {
      fprintf(MSG_OUT,
              "\nChanging action from %s to %s",
              whatstr[*actionp], whatstr[what]);
      setsock(actionp, s, e, what, *actionp, g);
    }
  }

  return 0;
}

/* CURLOPT_WRITEFUNCTION */
static size_t write_cb(void *ptr, size_t size, size_t nmemb, void *data)
{
  size_t written = size * nmemb;
  char *pBuffer = (char *)malloc(written + 1);

  strncpy(pBuffer, (const char *)ptr, written);
  pBuffer[written] = '\0';

  fprintf(MSG_OUT, "%s", pBuffer);

  free(pBuffer);

  return written;
}

/* CURLOPT_PROGRESSFUNCTION */
static int prog_cb(void *p, double dltotal, double dlnow, double ult,
                   double uln)
{
  ConnInfo *conn = (ConnInfo *)p;

  (void)ult;
  (void)uln;

  fprintf(MSG_OUT, "\nProgress: %s (%g/%g)", conn->url, dlnow, dltotal);
  fprintf(MSG_OUT, "\nProgress: %s (%g)", conn->url, ult);

  return 0;
}

/* CURLOPT_OPENSOCKETFUNCTION */
static curl_socket_t opensocket(void *clientp, curlsocktype purpose,
                                struct curl_sockaddr *address)
{
  fprintf(MSG_OUT, "\nopensocket :");

  curl_socket_t sockfd = CURL_SOCKET_BAD;

  /* restrict to IPv4 */
  if(purpose == CURLSOCKTYPE_IPCXN && address->family == AF_INET) {
    /* create a tcp socket object */
    boost::asio::ip::tcp::socket *tcp_socket =
      new boost::asio::ip::tcp::socket(io_service);

    /* open it and get the native handle*/
    boost::system::error_code ec;
    tcp_socket->open(boost::asio::ip::tcp::v4(), ec);

    if(ec) {
      /* An error occurred */
      std::cout << std::endl << "Couldn't open socket [" << ec << "][" <<
        ec.message() << "]";
      fprintf(MSG_OUT, "\nERROR: Returning CURL_SOCKET_BAD to signal error");
    }
    else {
      sockfd = tcp_socket->native_handle();
      fprintf(MSG_OUT, "\nOpened socket %d", sockfd);

      /* save it for monitoring */
      socket_map.insert(std::pair<curl_socket_t,
                        boost::asio::ip::tcp::socket *>(sockfd, tcp_socket));
    }
  }

  return sockfd;
}

/* CURLOPT_CLOSESOCKETFUNCTION */
static int close_socket(void *clientp, curl_socket_t item)
{
  fprintf(MSG_OUT, "\nclose_socket : %d", item);

  std::map<curl_socket_t, boost::asio::ip::tcp::socket *>::iterator it =
      socket_map.find(item);

  if(it != socket_map.end()) {
    delete it->second;
    socket_map.erase(it);
  }

  return 0;
}

/* Create a new easy handle, and add it to the global curl_multi */
static void new_conn(char *url, GlobalInfo *g)
{
  ConnInfo *conn;
  CURLMcode rc;

  conn = (ConnInfo *) calloc(1, sizeof(ConnInfo));

  conn->easy = curl_easy_init();
  if(!conn->easy) {
    fprintf(MSG_OUT, "\ncurl_easy_init() failed, exiting!");
    exit(2);
  }

  conn->global = g;
  conn->url = strdup(url);
  curl_easy_setopt(conn->easy, CURLOPT_URL, conn->url);
  curl_easy_setopt(conn->easy, CURLOPT_WRITEFUNCTION, write_cb);
  curl_easy_setopt(conn->easy, CURLOPT_WRITEDATA, &conn);
  curl_easy_setopt(conn->easy, CURLOPT_VERBOSE, 1L);
  curl_easy_setopt(conn->easy, CURLOPT_ERRORBUFFER, conn->error);
  curl_easy_setopt(conn->easy, CURLOPT_PRIVATE, conn);
  curl_easy_setopt(conn->easy, CURLOPT_NOPROGRESS, 1L);
  curl_easy_setopt(conn->easy, CURLOPT_PROGRESSFUNCTION, prog_cb);
  curl_easy_setopt(conn->easy, CURLOPT_PROGRESSDATA, conn);
  curl_easy_setopt(conn->easy, CURLOPT_LOW_SPEED_TIME, 3L);
  curl_easy_setopt(conn->easy, CURLOPT_LOW_SPEED_LIMIT, 10L);
  curl_easy_setopt(conn->easy, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2TLS);

  /* call this function to get a socket */
  curl_easy_setopt(conn->easy, CURLOPT_OPENSOCKETFUNCTION, opensocket);

  /* call this function to close a socket */
  curl_easy_setopt(conn->easy, CURLOPT_CLOSESOCKETFUNCTION, close_socket);

  fprintf(MSG_OUT,
          "\nAdding easy %p to multi %p (%s)", conn->easy, g->multi, url);
  rc = curl_multi_add_handle(g->multi, conn->easy);
  g->on_the_way++;
  mcode_or_die("new_conn: curl_multi_add_handle", rc);

  /* note that the add_handle() will set a time-out to trigger very soon so
     that the necessary socket_action() call will be called by this app */
}

int main(int argc, char **argv)
{
  GlobalInfo g;

  (void)argc;
  (void)argv;

  memset(&g, 0, sizeof(GlobalInfo));
  g.multi = curl_multi_init();
  g.on_the_way = 0;

  curl_multi_setopt(g.multi, CURLMOPT_SOCKETFUNCTION, sock_cb);
  curl_multi_setopt(g.multi, CURLMOPT_SOCKETDATA, &g);
  curl_multi_setopt(g.multi, CURLMOPT_TIMERFUNCTION, multi_timer_cb);
  curl_multi_setopt(g.multi, CURLMOPT_TIMERDATA, &g);
  curl_multi_setopt(g.multi, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);

  size_t r = 0;
  /* enter io_service run loop */
  do {
      if(g.on_the_way < 3)
          new_conn((char *)"https://www.google.com", &g);  /* add a URL */
      r = io_service.run_one();
  } while(r);

  curl_multi_cleanup(g.multi);

  fprintf(MSG_OUT, "\ndone.\n");

  return 0;
}
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 24, 2020

Valgrind reports invalid read in event_cb - ignore it (it's a bug in the example, btw

That feels really unreliable. Can we fix the example first?

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 24, 2020

I think I fixed it. Code below

#include <curl/curl.h>
#include <boost/asio.hpp>
#include <boost/bind.hpp>
#include <iostream>
#include <map>
#include <chrono>

#define MSG_OUT stdout

/* boost::asio related objects
 * using global variables for simplicity
 */
boost::asio::io_service io_service;
boost::asio::steady_timer timer(io_service);
struct socket_info
{
    socket_info(boost::asio::ip::tcp::socket * socket, int action)
        : socket(socket)
        , action(action)
        {}

    boost::asio::ip::tcp::socket *socket = nullptr;
    int action = 0;
};
std::map<curl_socket_t, socket_info> socket_map;

/* Global information, common to all connections */
typedef struct _GlobalInfo
{
  CURLM *multi;
  int still_running;
  int on_the_way;
} GlobalInfo;

/* Information associated with a specific easy handle */
typedef struct _ConnInfo
{
  CURL *easy;
  char *url;
  GlobalInfo *global;
  char error[CURL_ERROR_SIZE];
} ConnInfo;

static void timer_cb(const boost::system::error_code & error, GlobalInfo *g);

/* Update the event timer after curl_multi library calls */
static int multi_timer_cb(CURLM *multi, long timeout_ms, GlobalInfo *g)
{
  fprintf(MSG_OUT, "\nmulti_timer_cb: timeout_ms %ld", timeout_ms);

  /* cancel running timer */
  timer.cancel();

  if(timeout_ms > 0) {
    /* update timer */
    timer.expires_from_now(std::chrono::milliseconds(timeout_ms));
    timer.async_wait(boost::bind(&timer_cb, _1, g));
  }
  else if(timeout_ms == 0) {
    /* call timeout function immediately */
    boost::system::error_code error; /*success*/
    timer_cb(error, g);
  }

  return 0;
}

/* Die if we get a bad CURLMcode somewhere */
static void mcode_or_die(const char *where, CURLMcode code)
{
  if(CURLM_OK != code) {
    const char *s;
    switch(code) {
    case CURLM_CALL_MULTI_PERFORM:
      s = "CURLM_CALL_MULTI_PERFORM";
      break;
    case CURLM_BAD_HANDLE:
      s = "CURLM_BAD_HANDLE";
      break;
    case CURLM_BAD_EASY_HANDLE:
      s = "CURLM_BAD_EASY_HANDLE";
      break;
    case CURLM_OUT_OF_MEMORY:
      s = "CURLM_OUT_OF_MEMORY";
      break;
    case CURLM_INTERNAL_ERROR:
      s = "CURLM_INTERNAL_ERROR";
      break;
    case CURLM_UNKNOWN_OPTION:
      s = "CURLM_UNKNOWN_OPTION";
      break;
    case CURLM_LAST:
      s = "CURLM_LAST";
      break;
    default:
      s = "CURLM_unknown";
      break;
    case CURLM_BAD_SOCKET:
      s = "CURLM_BAD_SOCKET";
      fprintf(MSG_OUT, "\nERROR: %s returns %s", where, s);
      /* ignore this error */
      return;
    }

    fprintf(MSG_OUT, "\nERROR: %s returns %s", where, s);

    exit(code);
  }
}

/* Check for completed transfers, and remove their easy handles */
static void check_multi_info(GlobalInfo *g)
{
  char *eff_url;
  CURLMsg *msg;
  int msgs_left;
  ConnInfo *conn;
  CURL *easy;
  CURLcode res;

  fprintf(MSG_OUT, "\nREMAINING: %d", g->still_running);

  while((msg = curl_multi_info_read(g->multi, &msgs_left))) {
    if(msg->msg == CURLMSG_DONE) {
      easy = msg->easy_handle;
      res = msg->data.result;
      curl_easy_getinfo(easy, CURLINFO_PRIVATE, &conn);
      curl_easy_getinfo(easy, CURLINFO_EFFECTIVE_URL, &eff_url);
      fprintf(MSG_OUT, "\nDONE: %s => (%d) %s", eff_url, res, conn->error);
      curl_multi_remove_handle(g->multi, easy);
      free(conn->url);
      curl_easy_cleanup(easy);
      free(conn);
      g->on_the_way--;
    }
  }
}

/* Called by asio when there is an action on a socket */
static void event_cb(GlobalInfo *g, curl_socket_t s,
                     int action, const boost::system::error_code & error,
                     int *fdp)
{
  fprintf(MSG_OUT, "\nevent_cb: action=%d", action);

  if(socket_map.find(s) == socket_map.end()) {
    fprintf(MSG_OUT, "\nevent_cb: socket already closed");
    return;
  }

  /* make sure the event matches what are wanted */
  if(*fdp == action || *fdp == CURL_POLL_INOUT) {
    CURLMcode rc;
    if(error)
      action = CURL_CSELECT_ERR;
    rc = curl_multi_socket_action(g->multi, s, action, &g->still_running);

    mcode_or_die("event_cb: curl_multi_socket_action", rc);
    check_multi_info(g);

    if(g->still_running <= 0) {
      fprintf(MSG_OUT, "\nlast transfer done, kill timeout");
      timer.cancel();
    }

    /* keep on watching.
     * the socket may have been closed and/or fdp may have been changed
     * in curl_multi_socket_action(), so check them both */
    if(!error && socket_map.find(s) != socket_map.end() &&
       (*fdp == action || *fdp == CURL_POLL_INOUT)) {
      boost::asio::ip::tcp::socket *tcp_socket = socket_map.find(s)->second.socket;

      if(action == CURL_POLL_IN) {
        tcp_socket->async_read_some(boost::asio::null_buffers(),
                                    boost::bind(&event_cb, g, s,
                                                action, _1, fdp));
      }
      if(action == CURL_POLL_OUT) {
        tcp_socket->async_write_some(boost::asio::null_buffers(),
                                     boost::bind(&event_cb, g, s,
                                                 action, _1, fdp));
      }
    }
  }
}

/* Called by asio when our timeout expires */
static void timer_cb(const boost::system::error_code & error, GlobalInfo *g)
{
  if(!error) {
    fprintf(MSG_OUT, "\ntimer_cb: ");

    CURLMcode rc;
    rc = curl_multi_socket_action(g->multi, CURL_SOCKET_TIMEOUT, 0,
                                  &g->still_running);

    mcode_or_die("timer_cb: curl_multi_socket_action", rc);
    check_multi_info(g);
  }
}

/* Clean up any data */
static void remsock(int *f, GlobalInfo *g)
{
  fprintf(MSG_OUT, "\nremsock: ");
}

static void setsock(int *fdp, curl_socket_t s, CURL *e, int act, int oldact,
                    GlobalInfo *g)
{
  fprintf(MSG_OUT, "\nsetsock: socket=%d, act=%d, fdp=%p", s, act, fdp);

  std::map<curl_socket_t, socket_info>::iterator it =
    socket_map.find(s);

  if(it == socket_map.end()) {
    fprintf(MSG_OUT, "\nsocket %d is a c-ares socket, ignoring", s);
    return;
  }

  boost::asio::ip::tcp::socket * tcp_socket = it->second.socket;

  *fdp = act;

  if(act == CURL_POLL_IN) {
    fprintf(MSG_OUT, "\nwatching for socket to become readable");
    if(oldact != CURL_POLL_IN && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_read_some(boost::asio::null_buffers(),
                                  boost::bind(&event_cb, g, s,
                                              CURL_POLL_IN, _1, fdp));
    }
  }
  else if(act == CURL_POLL_OUT) {
    fprintf(MSG_OUT, "\nwatching for socket to become writable");
    if(oldact != CURL_POLL_OUT && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_write_some(boost::asio::null_buffers(),
                                   boost::bind(&event_cb, g, s,
                                               CURL_POLL_OUT, _1, fdp));
    }
  }
  else if(act == CURL_POLL_INOUT) {
    fprintf(MSG_OUT, "\nwatching for socket to become readable & writable");
    if(oldact != CURL_POLL_IN && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_read_some(boost::asio::null_buffers(),
                                  boost::bind(&event_cb, g, s,
                                              CURL_POLL_IN, _1, fdp));
    }
    if(oldact != CURL_POLL_OUT && oldact != CURL_POLL_INOUT) {
      tcp_socket->async_write_some(boost::asio::null_buffers(),
                                   boost::bind(&event_cb, g, s,
                                               CURL_POLL_OUT, _1, fdp));
    }
  }
}

static void addsock(curl_socket_t s, CURL *easy, int action, GlobalInfo *g)
{
  /* fdp is used to store current action */
  auto it = socket_map.find(s);
  if(it == socket_map.end())
      return;

  setsock(&(it->second.action), s, easy, action, 0, g);
  curl_multi_assign(g->multi, s, &(it->second.action));
}

/* CURLMOPT_SOCKETFUNCTION */
static int sock_cb(CURL *e, curl_socket_t s, int what, void *cbp, void *sockp)
{
  fprintf(MSG_OUT, "\nsock_cb: socket=%d, what=%d, sockp=%p", s, what, sockp);

  GlobalInfo *g = (GlobalInfo*) cbp;
  int *actionp = (int *) sockp;
  const char *whatstr[] = { "none", "IN", "OUT", "INOUT", "REMOVE"};

  fprintf(MSG_OUT,
          "\nsocket callback: s=%d e=%p what=%s ", s, e, whatstr[what]);

  if(what == CURL_POLL_REMOVE) {
    fprintf(MSG_OUT, "\n");
    remsock(actionp, g);
  }
  else {
    if(!actionp) {
      fprintf(MSG_OUT, "\nAdding data: %s", whatstr[what]);
      addsock(s, e, what, g);
    }
    else {
      fprintf(MSG_OUT,
              "\nChanging action from %s to %s",
              whatstr[*actionp], whatstr[what]);
      setsock(actionp, s, e, what, *actionp, g);
    }
  }

  return 0;
}

/* CURLOPT_WRITEFUNCTION */
static size_t write_cb(void *ptr, size_t size, size_t nmemb, void *data)
{
  size_t written = size * nmemb;
  char *pBuffer = (char *)malloc(written + 1);

  strncpy(pBuffer, (const char *)ptr, written);
  pBuffer[written] = '\0';

  fprintf(MSG_OUT, "%s", pBuffer);

  free(pBuffer);

  return written;
}

/* CURLOPT_PROGRESSFUNCTION */
static int prog_cb(void *p, double dltotal, double dlnow, double ult,
                   double uln)
{
  ConnInfo *conn = (ConnInfo *)p;

  (void)ult;
  (void)uln;

  fprintf(MSG_OUT, "\nProgress: %s (%g/%g)", conn->url, dlnow, dltotal);
  fprintf(MSG_OUT, "\nProgress: %s (%g)", conn->url, ult);

  return 0;
}

/* CURLOPT_OPENSOCKETFUNCTION */
static curl_socket_t opensocket(void *clientp, curlsocktype purpose,
                                struct curl_sockaddr *address)
{
  fprintf(MSG_OUT, "\nopensocket :");

  curl_socket_t sockfd = CURL_SOCKET_BAD;

  /* restrict to IPv4 */
  if(purpose == CURLSOCKTYPE_IPCXN && address->family == AF_INET) {
    /* create a tcp socket object */
    boost::asio::ip::tcp::socket *tcp_socket =
      new boost::asio::ip::tcp::socket(io_service);

    /* open it and get the native handle*/
    boost::system::error_code ec;
    tcp_socket->open(boost::asio::ip::tcp::v4(), ec);

    if(ec) {
      /* An error occurred */
      std::cout << std::endl << "Couldn't open socket [" << ec << "][" <<
        ec.message() << "]";
      fprintf(MSG_OUT, "\nERROR: Returning CURL_SOCKET_BAD to signal error");
    }
    else {
      sockfd = tcp_socket->native_handle();
      fprintf(MSG_OUT, "\nOpened socket %d", sockfd);

      /* save it for monitoring */
      socket_map.insert(std::pair<curl_socket_t,
                        socket_info>(sockfd, socket_info(tcp_socket, 0)));
    }
  }

  return sockfd;
}

/* CURLOPT_CLOSESOCKETFUNCTION */
static int close_socket(void *clientp, curl_socket_t item)
{
  fprintf(MSG_OUT, "\nclose_socket : %d", item);

  std::map<curl_socket_t, socket_info>::iterator it =
      socket_map.find(item);

  if(it != socket_map.end()) {
    delete it->second.socket;
    socket_map.erase(it);
  }

  return 0;
}

/* Create a new easy handle, and add it to the global curl_multi */
static void new_conn(char *url, GlobalInfo *g)
{
  ConnInfo *conn;
  CURLMcode rc;

  conn = (ConnInfo *) calloc(1, sizeof(ConnInfo));

  conn->easy = curl_easy_init();
  if(!conn->easy) {
    fprintf(MSG_OUT, "\ncurl_easy_init() failed, exiting!");
    exit(2);
  }

  conn->global = g;
  conn->url = strdup(url);
  curl_easy_setopt(conn->easy, CURLOPT_URL, conn->url);
  curl_easy_setopt(conn->easy, CURLOPT_WRITEFUNCTION, write_cb);
  curl_easy_setopt(conn->easy, CURLOPT_WRITEDATA, &conn);
  curl_easy_setopt(conn->easy, CURLOPT_VERBOSE, 1L);
  curl_easy_setopt(conn->easy, CURLOPT_ERRORBUFFER, conn->error);
  curl_easy_setopt(conn->easy, CURLOPT_PRIVATE, conn);
  curl_easy_setopt(conn->easy, CURLOPT_NOPROGRESS, 1L);
  curl_easy_setopt(conn->easy, CURLOPT_PROGRESSFUNCTION, prog_cb);
  curl_easy_setopt(conn->easy, CURLOPT_PROGRESSDATA, conn);
  curl_easy_setopt(conn->easy, CURLOPT_LOW_SPEED_TIME, 3L);
  curl_easy_setopt(conn->easy, CURLOPT_LOW_SPEED_LIMIT, 10L);
  curl_easy_setopt(conn->easy, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2TLS);

  /* call this function to get a socket */
  curl_easy_setopt(conn->easy, CURLOPT_OPENSOCKETFUNCTION, opensocket);

  /* call this function to close a socket */
  curl_easy_setopt(conn->easy, CURLOPT_CLOSESOCKETFUNCTION, close_socket);

  fprintf(MSG_OUT,
          "\nAdding easy %p to multi %p (%s)", conn->easy, g->multi, url);
  rc = curl_multi_add_handle(g->multi, conn->easy);
  g->on_the_way++;
  mcode_or_die("new_conn: curl_multi_add_handle", rc);

  /* note that the add_handle() will set a time-out to trigger very soon so
     that the necessary socket_action() call will be called by this app */
}

int main(int argc, char **argv)
{
  GlobalInfo g;

  (void)argc;
  (void)argv;

  memset(&g, 0, sizeof(GlobalInfo));
  g.multi = curl_multi_init();
  g.on_the_way = 0;

  curl_multi_setopt(g.multi, CURLMOPT_SOCKETFUNCTION, sock_cb);
  curl_multi_setopt(g.multi, CURLMOPT_SOCKETDATA, &g);
  curl_multi_setopt(g.multi, CURLMOPT_TIMERFUNCTION, multi_timer_cb);
  curl_multi_setopt(g.multi, CURLMOPT_TIMERDATA, &g);
  curl_multi_setopt(g.multi, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);

  size_t r = 0;
  /* enter io_service run loop */
  do {
      if(g.on_the_way < 3)
          new_conn((char *)"https://www.google.com", &g);  /* add a URL */
      r = io_service.run_one();
  } while(r);

  curl_multi_cleanup(g.multi);

  fprintf(MSG_OUT, "\ndone.\n");

  return 0;
}
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 24, 2020

Thanks!

I don't know anything about asio.

This code looks wrong - it uses the socket open and close callbacks to judge if the socket should be monitored. Creating and deleting sockets don't imply monitoring hints! I realize this is based on an example we provide but this is not a good use of our API. I'd even call it downright wrong. (subject for a separate issue if we can't work it out in here)

Can you make the example work without using CURLOPT_OPENSOCKETFUNCTION and CURLOPT_CLOSESOCKETFUNCTION ? It could also simplify the code.

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 24, 2020

This code looks wrong - it uses the socket open and close callbacks to judge if the socket should be monitored.

I think you were misleaded by the comment /* save it for monitoring */. But it doesn't imply actual monitoring. It only allocates boost::asio structures for socket and its monitoring. Actual monitoring begins after CURLMOPT_SOCKETFUNCTION is called.

Can you make the example work without using CURLOPT_OPENSOCKETFUNCTION and CURLOPT_CLOSESOCKETFUNCTION ?

No, I can't. It would be kind of plane without wings. As far as I understand curl and boost that is impossible.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 24, 2020

It would be kind of plane without wings. As far as I understand curl and boost that is impossible.

You're supposed to add/remove the socket to monitor based on the info in the socket callback. Why isn't that enough?

The special-handling of c-ares sockets in setsock() also shows why the current way is broken: it just ignores that socket.

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 24, 2020

You're supposed to add/remove the socket to monitor based on the info in the socket callback. Why isn't that enough?

We can't play with curl's socket handles if we want to make use of boost async functionality. For this we need to create boost::asio::ip::tcp::socket object associated with the raw socket handle upon open socket callback, and destroy it upon close socket callback. boost::asio::ip::tcp::socket is the key to use async functionality. Also we need to track last action which was supplied into setsock, because particular in boost asio we need to track how action changes between setsock calls.

The special-handling of c-ares sockets in setsock() also shows why the current way is broken: it just ignores that socket.

I agree that code looks ugly. But either you do know some other perfect way to use multi interface in async mode, or it is the way which multi interface actually works.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 24, 2020

We can't play with curl's socket handles if we want to make use of boost async functionality. For this we need to create boost::asio::ip::tcp::socket object associated with the raw socket handle upon open socket callback, and destroy it upon close socket callback

Why can't you call those functions and set that up when the socket callback tells you to monitor a socket and then also remove that socket again when the socket callback tells you to stop monitoring that socket? That's how the socket callback works, that's how it has always been intended to be used and that's how you use it with any other event library as well. What makes asio so special that you need to get pre-alerted that there's a socket coming?

in boost asio we need to track how action changes between setsock calls

Every event library needs to act on the new action in the new callback. That's why the callback is called for new actions.

I agree that code looks ugly

I said "broken" and I stand by it. It willfully ignores to monitor a socket (or more) that libcurl asks the application to monitor and therefore it will do badly then. That's worse than just ugly, it's wrong.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 24, 2020

I built the example and I've run it multiple times but it does not reproduce on my Debian Linux, libcurl from git master, libasio 1.12.2 (from a Debian Package). Tried with and without valgrind.

Does it repro every time for you? Should I keep trying?

@kunalekawde

This comment has been minimized.

Copy link

@kunalekawde kunalekawde commented Jan 27, 2020

Just wanted to update here that I'm facing similar issue with set of tests on 7.68.0 + 9607532

This was tracked in #4779

Thread 0 (crashed)
0  libBaseFramework.so!ConnectionExists [url.c : 1193 + 0x0]
1  libBaseFramework.so!create_conn [url.c : 3602 + 0x1f]
2  libBaseFramework.so!Curl_connect [url.c : 3853 + 0x17]
3  libBaseFramework.so!multi_runsingle [multi.c : 1626 + 0x20]
4  libBaseFramework.so!multi_socket [multi.c : 2811 + 0x1b]
5  libBaseFramework.so!curl_multi_socket_action [multi.c : 2934 + 0x1e

I'm trying to reproduce this and once I have some substantial information, I shall update -- may be as a different issue. Just for reference here.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 27, 2020

That certainly looks like a very similar stack trace so it might very well be the same problem.

It would be helpful if (both of you) could:

  1. use libcurl from current git master to make line numbers and code match the latest
  2. build with debug symbols present so that we get better stack traces in case of crashes

It looks like it is the check->data pointer that is invalid at the time a new connection is being setup. If so, that's an existing connection that has a bad transfer pointer. ->data should either point to a valid Curl_easy struct or be NULL.

So detailed observations about current and past set of transfers at the time of the crash is very important. How come this connection gets a broken transfer-pointer. How did the specific transfer that used that connection end etc?

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 27, 2020

In a debug build of curl, maybe the following can help to catch the problem sooner:

diff --git a/lib/url.c b/lib/url.c
index 689668e04..837e62a09 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1211,10 +1211,12 @@ ConnectionExists(struct Curl_easy *data,
               continue;
           }
         }
       }
 
+      DEBUGASSERT(!check->data || GOOD_EASY_HANDLE(check->data));
+
       if(!canmultiplex && check->data)
         /* this request can't be multiplexed but the checked connection is
            already in use so we skip it */
         continue;
 
@kunalekawde

This comment has been minimized.

Copy link

@kunalekawde kunalekawde commented Jan 27, 2020

In a debug build of curl, maybe the following can help to catch the problem sooner:

diff --git a/lib/url.c b/lib/url.c
index 689668e04..837e62a09 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1211,10 +1211,12 @@ ConnectionExists(struct Curl_easy *data,
               continue;
           }
         }
       }
 
+      DEBUGASSERT(!check->data || GOOD_EASY_HANDLE(check->data));
+
       if(!canmultiplex && check->data)
         /* this request can't be multiplexed but the checked connection is
            already in use so we skip it */
         continue;
 

Sure, shall try with git master and above

@kunalekawde

This comment has been minimized.

Copy link

@kunalekawde kunalekawde commented Jan 27, 2020

yes it hits at assertion

#0  0x00007f6ca1c51feb in raise () from /lib64/libc.so.6
#1  0x00007f6ca1c3c5c1 in abort () from /lib64/libc.so.6
#2  0x00007f6ca1c3c491 in __assert_fail_base.cold.0 () from /lib64/libc.so.6
#3  0x00007f6ca1c4a752 in __assert_fail () from /lib64/libc.so.6
#4  0x00007f6ca9fdb3a6 in ConnectionExists (data=0x28412e0, needle=0x2846b00, usethis=0x7ffcf971ba88, force_reuse=0x7ffcf971ba87, waitpipe=0x7ffcf971ba86) at url.c:1216
#5  0x00007f6ca9fe026a in create_conn (data=0x28412e0, in_connect=0x7ffcf971bb00, async=0x7ffcf971bb76) at url.c:3624
#6  0x00007f6ca9fe09bf in Curl_connect (data=0x28412e0, asyncp=0x7ffcf971bb76, protocol_done=0x7ffcf971bb75) at url.c:3875
#7  0x00007f6ca9fa5efa in multi_runsingle (multi=0x24f5310, now=..., data=0x28412e0) at multi.c:1629
#8  0x00007f6ca9fa84b2 in multi_socket (multi=0x24f5310, checkall=false, s=19, ev_bitmask=0, running_handles=0x7ffcf971bea0) at multi.c:2814
#9  0x00007f6ca9fa8c36 in curl_multi_socket_action (multi=0x24f5310, s=19, ev_bitmask=0, running_handles=0x7ffcf971bea0) at multi.c:2937
(gdb) p needle->data->multi
$5 = (struct Curl_multi *) 0x24f5310
(gdb) p check->data->multi
$6 = (struct Curl_multi *) 0x0

(gdb) p *check->data
...
  **magic = 320**

https://gist.github.com/kunalekawde/59a6e3c7a737b36bc9502c25b16a8373

@kunalekawde

This comment has been minimized.

Copy link

@kunalekawde kunalekawde commented Jan 27, 2020

I cannot say with confirmation but git latest + assertion, its hitting very soon, earlier related crash was seen a little later.
Few details on what is used:

  1. curl_share_setopt(curlSharedHandle_m, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS); --> with defined lock/unlock
  2. curl_easy_setopt(handle_mp, CURLOPT_DNS_CACHE_TIMEOUT, timeout);
  3. socket and timer callbacks
  4. limits:
    curl_multi_setopt(multi_handle_m, CURLMOPT_MAXCONNECTS, maxConnects);
    curl_multi_setopt(multi_handle_m, CURLMOPT_MAX_HOST_CONNECTIONS, maxConnectsPerHost);
    curl_multi_setopt(multi_handle_m, CURLMOPT_MAX_TOTAL_CONNECTIONS, maxParallelConnects);
    curl_multi_setopt(multi_handle_m, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);
    curl_multi_setopt(multi_handle_m, CURLMOPT_MAX_CONCURRENT_STREAMS, 200L);
  5. curl_easy_setopt(GetHandle(), CURLOPT_PIPEWAIT, 1L);

I could also get valgrind for same:

==46== Invalid read of size 4
==46==    at 0x5F0C37A: ConnectionExists (url.c:1216)
==46==    by 0x5F11269: create_conn (url.c:3624)
==46==    by 0x5F119BE: Curl_connect (url.c:3875)
==46==    by 0x5ED6EF9: multi_runsingle (multi.c:1629)
==46==    by 0x5ED94B1: multi_socket (multi.c:2814)
==46==    by 0x5ED9C35: curl_multi_socket_action (multi.c:2937)

Probably check->data was freed earlier -- this would happen on response:

==46==  Address 0x1b587360 is 6,016 bytes inside a block of size 6,024 free'd
==46==    at 0x4C2FDAC: free (vg_replace_malloc.c:530)
==46==    by 0x5F0A803: Curl_close (url.c:415)
==46==    by 0x5ED0B81: curl_easy_cleanup (easy.c:746)
==46==    by 0x5EBFC59: HttpRequestBase::~HttpRequestBase() (httpRequestBase.cc:51)

which was allocated -- while sending request

==46==  Block was alloc'd at
==46==    at 0x4C30B06: calloc (vg_replace_malloc.c:711)
==46==    by 0x5F0ADF8: Curl_open (url.c:584)
==46==    by 0x5ED0832: curl_easy_init (easy.c:306)
==46==    by 0x5EBF7E9: HttpRequestBase::HttpRequestBase() (httpRequestBase.cc:40)
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 27, 2020

There's no need to start spewing stack traces here for when the assert or valgrind problems trigger. We already know what's wrong then. We need to figure out how the problem is introduced. Why is the conn->data pointer not cleared when the easy handle is freed.

@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 27, 2020

Does it repro every time for you? Should I keep trying?

For me it happens every time when I run example. Just a couple of seconds.

but it does not reproduce on my Debian Linux, libcurl from git master, libasio 1.12.2 (from a Debian Package)

Have you built it with libnghttp2? Key point that it reproduces only when HTTP/2 is being used.

Why is the conn->data pointer not cleared when the easy handle is freed.

There is really no code which checks during curl_multi_remove_handle() if easy handle is bundled with any connection and checks it conn->data. Get back to this message #4845 (comment) - here is the place where we end up with some conn->data assigned, but there is no other place in code (i haven't found) where conn->data is cleared or assigned to another easy handle.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 27, 2020

Have you built it with libnghttp2? Key point that it reproduces only when HTTP/2 is being used.

Yes I did. Possibly that c-ares flaw haunted my attempts. I'll try a build without c-ares.

There is really no code which checks during curl_multi_remove_handle() if easy handle is bundled with any connection and checks it conn->data.

In the curl_multi_remove_handle function, if there's still a connection associated with the transfer:

curl/lib/multi.c

Lines 772 to 773 in 872ea75

if(data->conn)
detach_connnection(data);

That removes the connection from the easy handle to the connection.

The other direction is cleared in multi_done():

conn->data = NULL; /* the connection now has no owner */

Where the connection's 'data' pointer is cleared after the transfer is "done".

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 27, 2020

The example crash reproduces for me with a clang sanitizer build with the threaded resolver.

bagder added a commit that referenced this issue Jan 27, 2020
... since the current transfer is being killed. Setting to NULL is
wrong, leaving it pointing to 'data' is wrong since that handle might be
about to get freed.

Fixes #4845
Reported-by: dmitrmax on github
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 27, 2020

#4858 made the issue not appear for me anymore.

@bagder bagder changed the title valgrind invalid read invalid `conn->data` pointer in ConnectionExists when multiplexing Jan 28, 2020
@kunalekawde

This comment has been minimized.

Copy link

@kunalekawde kunalekawde commented Jan 28, 2020

Yeah this seems to be working for my test run as well.

@bagder bagder closed this in db9af34 Jan 28, 2020
@dmitrmax

This comment has been minimized.

Copy link
Author

@dmitrmax dmitrmax commented Jan 28, 2020

Thanks, guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.