[ovs-dev] [PATCH] ovs-ofctl: Avoid use-after-free upon "ofctl/unblock" when connection dies.

Ansis Atteka aatteka at nicira.com
Thu Jul 12 23:30:13 UTC 2012


On Thu, Jul 12, 2012 at 4:05 PM, Ben Pfaff <blp at nicira.com> wrote:

> The implementation of "ofctl/block" used a nested poll loop, with an inner
> call to unixctl_server_run().  This poll loop always ran inside an outer
> call to unixctl_server_run(), since that's the context within which unixctl
> command implementations run.  That means that, if a unixctl connection got
> closed within the inner poll loop, and the outer poll loop happened to be
> processing the same unixctl connection, then the outer poll loop would
> dereference data in the freed connection.
>
> The simplest solution is to avoid a nested poll loop, so that's what this
> commit does.
>
> This didn't cause a failure in the unit tests on i386 (which is why I
> didn't catch it before pushing) but it did, reliably, on x86-64, and it
> showed up in valgrind everywhere.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  utilities/ovs-ofctl.c |   59
> ++++++++++++++++--------------------------------
>  1 files changed, 20 insertions(+), 39 deletions(-)
>
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index d633d1c..00cdb5f 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -1256,49 +1256,31 @@ ofctl_set_output_file(struct unixctl_conn *conn,
> int argc OVS_UNUSED,
>      unixctl_command_reply(conn, NULL);
>  }
>
> -struct block_aux {
> -    struct vconn *vconn;
> -    struct unixctl_server *server;
> -    bool blocked;
> -};
> -
>  static void
>  ofctl_block(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -            const char *argv[] OVS_UNUSED, void *block_)
> +            const char *argv[] OVS_UNUSED, void *blocked_)
>  {
> -    struct block_aux *block = block_;
> +    bool *blocked = blocked_;
>
> -    if (block->blocked) {
> +    if (!*blocked) {
> +        *blocked = true;
> +        unixctl_command_reply(conn, NULL);
> +    } else {
>          unixctl_command_reply(conn, "already blocking");
> -        return;
> -    }
> -
> -    block->blocked = true;
> -    unixctl_command_reply(conn, NULL);
> -    for (;;) {
> -        unixctl_server_run(block->server);
> -        if (!block->blocked) {
> -            break;
> -        }
> -        vconn_run(block->vconn);
> -
> -        unixctl_server_wait(block->server);
> -        vconn_run_wait(block->vconn);
> -        poll_block();
>      }
>  }
>
>  static void
>  ofctl_unblock(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -              const char *argv[] OVS_UNUSED, void *block_)
> +              const char *argv[] OVS_UNUSED, void *blocked_)
>  {
> -    struct block_aux *block = block_;
> +    bool *blocked = blocked_;
>
> -    if (!block->blocked) {
> -        unixctl_command_reply(conn, "not blocking");
> -    } else {
> -        block->blocked = false;
> +    if (*blocked) {
> +        *blocked = false;
>          unixctl_command_reply(conn, NULL);
> +    } else {
> +        unixctl_command_reply(conn, "not blocking");
>
Shouldn't this be "not unblocking"? maybe I am missing something....

>      }
>  }
>
> @@ -1306,9 +1288,9 @@ static void
>  monitor_vconn(struct vconn *vconn)
>  {
>      struct barrier_aux barrier_aux = { vconn, NULL };
> -    struct block_aux block;
>      struct unixctl_server *server;
>      bool exiting = false;
> +    bool blocked = false;
>      int error;
>
>      daemon_save_fd(STDERR_FILENO);
> @@ -1325,11 +1307,9 @@ monitor_vconn(struct vconn *vconn)
>      unixctl_command_register("ofctl/set-output-file", "FILE", 1, 1,
>                               ofctl_set_output_file, NULL);
>
> -    block.vconn = vconn;
> -    block.server = server;
> -    block.blocked = false;
> -    unixctl_command_register("ofctl/block", "", 0, 0, ofctl_block,
> &block);
> -    unixctl_command_register("ofctl/unblock", "", 0, 0, ofctl_unblock,
> &block);
> +    unixctl_command_register("ofctl/block", "", 0, 0, ofctl_block,
> &blocked);
> +    unixctl_command_register("ofctl/unblock", "", 0, 0, ofctl_unblock,
> +                             &blocked);
>
>      daemonize_complete();
>
> @@ -1338,8 +1318,7 @@ monitor_vconn(struct vconn *vconn)
>          int retval;
>
>          unixctl_server_run(server);
> -
> -        for (;;) {
> +        while (!blocked) {
>              uint8_t msg_type;
>
>              retval = vconn_recv(vconn, &b);
> @@ -1372,7 +1351,9 @@ monitor_vconn(struct vconn *vconn)
>
>          vconn_run(vconn);
>          vconn_run_wait(vconn);
> -        vconn_recv_wait(vconn);
> +        if (!blocked) {
> +            vconn_recv_wait(vconn);
> +        }
>          unixctl_server_wait(server);
>          poll_block();
>      }
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
Other than that fixes the build for me and looks good.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120712/ef95b1df/attachment-0003.html>


More information about the dev mailing list