Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect

[Orit Wasserman <owasserm@redhat.com>]

On 09/21/2012 11:03 AM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> On 09/20/2012 04:14 PM, Markus Armbruster wrote:
>>> Orit Wasserman <owasserm@redhat.com> writes:
>>>
>>>> getaddrinfo can give us a list of addresses, but we only try to
>>>> connect to the first one. If that fails we never proceed to
>>>> the next one.  This is common on desktop setups that often have ipv6
>>>> configured but not actually working.
>>>>
>>>> To fix this make inet_connect_nonblocking retry connection with a different
>>>> address.
>>>> callers on inet_nonblocking_connect register a callback function that will
>>>> be called when connect opertion completes, in case of failure the fd will have
>>>> a negative value
>>>>
>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>  migration-tcp.c |   29 +++++-----------
>>>>  qemu-char.c     |    2 +-
>>>>  qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  qemu_socket.h   |   13 ++++++--
>>>>  4 files changed, 102 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/migration-tcp.c b/migration-tcp.c
>>>> index 7f6ad98..cadea36 100644
>>>> --- a/migration-tcp.c
>>>> +++ b/migration-tcp.c
>>>> @@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
>>>>      return r;
>>>>  }
>>>>  
>>>> -static void tcp_wait_for_connect(void *opaque)
>>>> +static void tcp_wait_for_connect(int fd, void *opaque)
>>>>  {
>>>>      MigrationState *s = opaque;
>>>> -    int val, ret;
>>>> -    socklen_t valsize = sizeof(val);
>>>>  
>>>> -    DPRINTF("connect completed\n");
>>>> -    do {
>>>> -        ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>>>> -    } while (ret == -1 && (socket_error()) == EINTR);
>>>> -
>>>> -    if (ret < 0) {
>>>> +    if (fd < 0) {
>>>> +        DPRINTF("migrate connect error\n");
>>>> +        s->fd = -1;
>>>>          migrate_fd_error(s);
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> -
>>>> -    if (val == 0)
>>>> +    } else {
>>>> +        DPRINTF("migrate connect success\n");
>>>> +        s->fd = fd;
>>>>          migrate_fd_connect(s);
>>>> -    else {
>>>> -        DPRINTF("error connecting %d\n", val);
>>>> -        migrate_fd_error(s);
>>>>      }
>>>>  }
>>>>  
>>>> @@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>>>>      s->write = socket_write;
>>>>      s->close = tcp_close;
>>>>  
>>>> -    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
>>>> +    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
>>>> +                                     &in_progress, errp);
>>>>      if (error_is_set(errp)) {
>>>>          migrate_fd_error(s);
>>>>          return -1;
>>>> @@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>>>>  
>>>>      if (in_progress) {
>>>>          DPRINTF("connect in progress\n");
>>>> -        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>>>>      } else {
>>>>          migrate_fd_connect(s);
>>>>      }
>>>> diff --git a/qemu-char.c b/qemu-char.c
>>>> index c442952..11cd5ef 100644
>>>> --- a/qemu-char.c
>>>> +++ b/qemu-char.c
>>>> @@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>>>>          if (is_listen) {
>>>>              fd = inet_listen_opts(opts, 0, NULL);
>>>>          } else {
>>>> -            fd = inet_connect_opts(opts, true, NULL, NULL);
>>>> +            fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
>>>>          }
>>>>      }
>>>>      if (fd < 0) {
>>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>>> index 212075d..d321c58 100644
>>>> --- a/qemu-sockets.c
>>>> +++ b/qemu-sockets.c
>>>> @@ -24,6 +24,7 @@
>>>>  
>>>>  #include "qemu_socket.h"
>>>>  #include "qemu-common.h" /* for qemu_isdigit */
>>>> +#include "main-loop.h"
>>>>  
>>>>  #ifndef AI_ADDRCONFIG
>>>>  # define AI_ADDRCONFIG 0
>>>> @@ -217,11 +218,69 @@ listen:
>>>>      ((rc) == -EINPROGRESS)
>>>>  #endif
>>>>  
>>>> +/* Struct to store connect state for non blocking connect */
>>>> +typedef struct ConnectState {
>>>> +    int fd;
>>>> +    struct addrinfo *addr_list;
>>>> +    struct addrinfo *current_addr;
>>>> +    ConnectHandler *callback;
>>>> +    void *opaque;
>>>> +    Error *errp;
>>>> +} ConnectState;
>>>> +
>>>>  static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>> -                             bool *in_progress)
>>>> +                             bool *in_progress, ConnectState *connect_state);
>>>> +
>>>> +static void wait_for_connect(void *opaque)
>>>> +{
>>>> +    ConnectState *s = opaque;
>>>> +    int val = 0, rc = 0;
>>>> +    socklen_t valsize = sizeof(val);
>>>> +    bool in_progress = false;
>>>> +
>>>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> +
>>>> +    do {
>>>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>>>> +    } while (rc == -1 && socket_error() == EINTR);
>>>> +
>>>> +    /* update rc to contain error details */
>>>> +    if (!rc && val) {
>>>> +        rc = -val;
>>>
>>> Would rc = -1 suffice?  I'd find that clearer.
>> I guess so, I want the errno for more detailed error message
>> but those will come in another patch set and I can handle it than.
>> I agree that using -1 will make the code much cleaner.
>>>
>>>> +    }
>>>> +
>>>> +    /* connect error */
>>>> +    if (rc < 0) {
>>>> +        closesocket(s->fd);
>>>> +        s->fd = rc;
>>>> +    }
>>>> +
>>>> +    /* try to connect to the next address on the list */
>>>> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
>>>> +        s->current_addr = s->current_addr->ai_next;
>>>> +        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
>>>> +        /* connect in progress */
>>>> +        if (in_progress) {
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    freeaddrinfo(s->addr_list);
>>>> +    if (s->callback) {
>>>> +        s->callback(s->fd, s->opaque);
>>>> +    }
>>>> +    g_free(s);
>>>> +    return;
>>>> +}
>>>> +
>>>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>> +                             bool *in_progress, ConnectState *connect_state)
>>>
>>> connect_state is needed only for non-blocking connect, isn't it?  Could
>>> we drop block and simply use connect_state == NULL instead?
>> it is a good idea !
>>>
>>>>  {
>>>>      int sock, rc;
>>>>  
>>>> +    if (in_progress) {
>>>> +        *in_progress = false;
>>>> +    }
>>>>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>>>>      if (sock < 0) {
>>>>          fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>>>> @@ -241,6 +300,9 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>>      } while (rc == -EINTR);
>>>>  
>>>>      if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>>>> +        connect_state->fd = sock;
>>>> +        qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
>>>> +                             connect_state);
>>>>          if (in_progress) {
>>>>              *in_progress = true;
>>>>          }
>>>> @@ -259,6 +321,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>>      const char *port;
>>>>  
>>>>      memset(&ai, 0, sizeof(ai));
>>>> +
>>>>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>>>>      ai.ai_family = PF_UNSPEC;
>>>>      ai.ai_socktype = SOCK_STREAM;
>>>> @@ -291,7 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>>  }
>>>>  
>>>>  int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>>> -                      Error **errp)
>>>> +                      Error **errp, ConnectState *connect_state)
>>>
>>> Same as for inet_connect_addr(): could we drop block and simply use
>>> connect_state == NULL instead?
>>>
>>>>  {
>>>>      struct addrinfo *res, *e;
>>>>      int sock = -1;
>>>> @@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>>>          return -1;
>>>>      }
>>>>  
>>>> -    if (in_progress) {
>>>> -        *in_progress = false;
>>>> -    }
>>>> -
>>>>      for (e = res; e != NULL; e = e->ai_next) {
>>>> -        sock = inet_connect_addr(e, block, in_progress);
>>>> -        if (sock >= 0) {
>>>> +        if (!block) {
>>>> +            connect_state->addr_list = res;
>>>> +            connect_state->current_addr = e;
>>>> +        }
>>>> +        sock = inet_connect_addr(e, block, in_progress, connect_state);
>>>> +        if (in_progress && *in_progress) {
>>>> +            return sock;
>>>> +        } else if (sock >= 0) {
>>>>              break;
>>>>          }
>>>>      }
>>>>      if (sock < 0) {
>>>>          error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>>>      }
>>>
>>> Testing in_progress is fishy: whether the caller passes in_progress or
>>> not shouldn't affect what this function does, only how it reports what
>>> it did.
>>>
>>> inet_connect_addr() either
>>>
>>> 1. completes connect (returns valid fd, sets *in_progress to false), or
>>>
>>> 2. starts connect (returns valid fd, sets *in_progress to true), or
>>>
>>> 3. fails (returns -1 and sets *in_progress to false).
>>>
>>> In case 3, we want to try the next address.  If there is none, fail.
>>>
>>> In cases 1 and 2, we want to break the loop and return sock.
>>>
>>> What about:
>>>
>>>     for (e = res; sock < 0 && e != NULL; e = e->ai_next) {
>>>         if (!block) {
>>>             connect_state->addr_list = res;
>>>             connect_state->current_addr = e;
>>>         }
>>>         sock = inet_connect_addr(e, block, in_progress, connect_state);
>>>     }
>>>     if (sock < 0) {
>>>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>>     }
>>>     freeaddrinfo(res);
>>>     return sock;
>>>
>>>> +    g_free(connect_state);
>>>
>>> connect_state isn't allocated in this function, are you sure you want to
>>> free it here?  If yes, are you sure you want to free it only sometimes?
>>>
>> inet_nonblocking connect allocate connect_state.
>> In case connect succeeded/failed immediately than we need to free it
>> (i.e in_progress is true),
>> that the case here.
>> I will move it to inet_nonblocking_connect when I remove in_progress flag.
>>
>> We still need to free in two places in the code ,the second is in
>> tcp_wait_for_connect.
> 
> Do you mean wait_for_connect()?
> 
> Here's how I now think it's designed to work: inet_connect_opts() frees
> connect_state.  Except when connect_state->callback is going to be
> called, it's freed only after the callback returns.  In no case, the
> caller or its callback function need to free it.
> 
> In short, inet_connect_opts()'s caller only allocates, the free is
> automatic.  Needs mention in function comment.
> 
> Ways to avoid splitting alloc/free responsibility between
> inet_connect_opts() and its callers:
> 
> 1. Move free into caller code.  Probably a bad idea, because it needs to
> be done by the caller when in_progress, else by the callback, which
> feels error-prone.
> 
> 2. Move allocation into inet_connect_opts(), replace connect_state
> argument by whatever the caller needs put in connect_state.  I'm not
> saying you should do this, just throwing out the idea; use it if you
> like it.
> 
>>>>      freeaddrinfo(res);
>>>>      return sock;
>>>>  }
>>>> @@ -518,7 +584,7 @@ int inet_connect(const char *str, Error **errp)
>>>>  
>>>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>>      if (inet_parse(opts, str) == 0) {
>>>> -        sock = inet_connect_opts(opts, true, NULL, errp);
>>>> +        sock = inet_connect_opts(opts, true, NULL, errp, NULL);
>>>>      } else {
>>>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>>>      }
>>>> @@ -526,16 +592,19 @@ int inet_connect(const char *str, Error **errp)
>>>>      return sock;
>>>>  }
>>>>  
>>>> -
>>>> -int inet_nonblocking_connect(const char *str, bool *in_progress,
>>>> -                             Error **errp)
>>>> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
>>>> +                             void *opaque, bool *in_progress, Error **errp)
>>>>  {
>>>>      QemuOpts *opts;
>>>>      int sock = -1;
>>>> +    ConnectState *connect_state;
>>>>  
>>>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>>      if (inet_parse(opts, str) == 0) {
>>>> -        sock = inet_connect_opts(opts, false, in_progress, errp);
>>>> +        connect_state = g_malloc0(sizeof(*connect_state));
>>>> +        connect_state->callback = callback;
>>>> +        connect_state->opaque = opaque;
>>>> +        sock = inet_connect_opts(opts, false, in_progress, errp, connect_state);
>>>
>>> May leak connect_state, because inet_connect_opts() doesn't always free
>>> it.
>> it is freed by tcp_wait_for_connect after it calls the user callback function
> 
> wait_for_connect(), actually.  You're right.
> 
> Now I actually understand how this works, let me have another try at
> pointing out allocation errors:
> 
>     int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>                           Error **errp, ConnectState *connect_state)
>     {
>         struct addrinfo *res, *e;
>         int sock = -1;
> 
>         res = inet_parse_connect_opts(opts, errp);
>         if (!res) {
> 
> Leaks connect_state.
right missed it, I will add a free here
> 
>             return -1;
>         }
> 
>         for (e = res; e != NULL; e = e->ai_next) {
>             if (!block) {
>                 connect_state->addr_list = res;
>                 connect_state->current_addr = e;
>             }
>             sock = inet_connect_addr(e, block, in_progress, connect_state);
>             if (in_progress && *in_progress) {
> 
> If inet_connect_addr() only started the connect, it arranged for
> wait_for_connect() to run when the connect completes.
> wait_for_connect() frees connect_state.
> 
> If in_progress, we detect this correctly, and refrain from freeing
> connect_state.
> 
> If !in_progress, we fail to detect it, and free connect_state().  When
> wait_for_connect() runs, we get a use-after-free, and a double-free.
> 
> Possible fixes:
> 
> 1. Document caller must pass non-null in_progress for non-blocking
> connect.  Recommend assert().
> 
> 2. Stick "if (!in_progress) in_progress = &local_in_progress;" before
> the loop.
> 
> 3. Move the free into caller code, as described above (still not
> thrilled about that idea).
> 
> I'd recommend 1. if you think passing null in_progress for non-blocking
> connect makes no sense.  I doubt that's the case.
> 
> Else I'd recommend 2.
> 
>                 return sock;
>             } else if (sock >= 0) {
>                 break;
>             }
>         }
>         if (sock < 0) {
>             error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>         }
>         g_free(connect_state);
>         freeaddrinfo(res);
>         return sock;
>     }
> 
> 
> [...]
> 
Well I removed in_progress from inet_nonblocking_connect as mst requested, this actually helps with 
this issue as it becomes a local var in inet_connect_opts.

Orit