On Thu, Jul 12, 2012 at 4:05 PM, Ben Pfaff <b...@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 <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
Other than that fixes the build for me and looks good.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
