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

[Markus Armbruster]

Orit Wasserman <address@hidden> writes:

> On 09/20/2012 04:14 PM, Markus Armbruster wrote:
>> Orit Wasserman <address@hidden> 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 <address@hidden>
>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>> ---
>>>  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.

            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;
    }


[...]