[ovs-dev] [PATCH] stream-unix: append ovs_rundir to socket

Ben Pfaff blp at nicira.com
Fri Feb 8 19:18:38 UTC 2013


Yes, would you mind taking a first shot?

On Fri, Feb 08, 2013 at 09:51:33AM -0800, Pavithra Ramesh wrote:
> Thanks Ben. The patch looks good.
> Do we also want to change the python stream implementation
> to behave the same way? 
> 
> -Pavithra
> 
> ----- Original Message -----
> From: "Ben Pfaff" <blp at nicira.com>
> To: "Pavithra Ramesh" <paramesh at vmware.com>
> Cc: dev at openvswitch.org
> Sent: Friday, February 8, 2013 9:14:13 AM
> Subject: Re: [ovs-dev] [PATCH] stream-unix: append ovs_rundir to socket
> 
> On Thu, Feb 07, 2013 at 03:48:14PM -0800, Pavithra Ramesh wrote:
> > If socket path specified is relative to ovs_rundir(),
> > append the directory name to in unix_open and punix_open.
> > Freed the new newly allocated strings.
> > Also included the change in bridge.c to relax the whitelist
> > check, only if there is no /.
> > 
> > Signed-off-by: Pavithra Ramesh <paramesh at vmware.com>
> 
> Here's a revised version that passes all tests.
> 
> --8<--------------------------cut here-------------------------->8--
> 
> From: Pavithra Ramesh <paramesh at vmware.com>
> Date: Thu, 7 Feb 2013 15:48:14 -0800
> Subject: [PATCH] stream-unix: append ovs_rundir to socket
> 
> If socket path specified is relative to ovs_rundir(),
> append the directory name to in unix_open and punix_open.
> Freed the new newly allocated strings.
> Also included the change in bridge.c to relax the whitelist
> check, only if there is no /.
> 
> Signed-off-by: Pavithra Ramesh <paramesh at vmware.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/stream-unix.c        |   20 ++++++++++++++------
>  tests/jsonrpc.at         |    3 +++
>  tests/ovsdb-execution.at |    1 +
>  tests/ovsdb-idl.at       |    3 +++
>  tests/ovsdb-macros.at    |    3 ++-
>  tests/ovsdb-monitor.at   |    1 +
>  tests/ovsdb-server.at    |    9 +++++++++
>  tests/ovsdb-tool.at      |    4 ++++
>  tests/vconn.at           |    1 +
>  vswitchd/bridge.c        |    6 ++++--
>  10 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/stream-unix.c b/lib/stream-unix.c
> index 6ed7648..dbee135 100644
> --- a/lib/stream-unix.c
> +++ b/lib/stream-unix.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -29,6 +29,7 @@
>  #include "packets.h"
>  #include "poll-loop.h"
>  #include "socket-util.h"
> +#include "dirs.h"
>  #include "util.h"
>  #include "stream-provider.h"
>  #include "stream-fd.h"
> @@ -42,15 +43,19 @@ static int
>  unix_open(const char *name, char *suffix, struct stream **streamp,
>            uint8_t dscp OVS_UNUSED)
>  {
> -    const char *connect_path = suffix;
> +    char *connect_path;
>      int fd;
>  
> +    connect_path = abs_file_name(ovs_rundir(), suffix);
>      fd = make_unix_socket(SOCK_STREAM, true, NULL, connect_path);
> +
>      if (fd < 0) {
>          VLOG_DBG("%s: connection failed (%s)", connect_path, strerror(-fd));
> +        free(connect_path);
>          return -fd;
>      }
>  
> +    free(connect_path);
>      return new_fd_stream(name, fd, check_connection_completion(fd), streamp);
>  }
>  
> @@ -76,11 +81,14 @@ static int
>  punix_open(const char *name OVS_UNUSED, char *suffix,
>             struct pstream **pstreamp, uint8_t dscp OVS_UNUSED)
>  {
> +    char *bind_path;
>      int fd, error;
>  
> -    fd = make_unix_socket(SOCK_STREAM, true, suffix, NULL);
> +    bind_path = abs_file_name(ovs_rundir(), suffix);
> +    fd = make_unix_socket(SOCK_STREAM, true, bind_path, NULL);
>      if (fd < 0) {
> -        VLOG_ERR("%s: binding failed: %s", suffix, strerror(errno));
> +        VLOG_ERR("%s: binding failed: %s", bind_path, strerror(errno));
> +        free(bind_path);
>          return errno;
>      }
>  
> @@ -88,11 +96,11 @@ punix_open(const char *name OVS_UNUSED, char *suffix,
>          error = errno;
>          VLOG_ERR("%s: listen: %s", name, strerror(error));
>          close(fd);
> +        free(bind_path);
>          return error;
>      }
>  
> -    return new_fd_pstream(name, fd, punix_accept, NULL,
> -                          xstrdup(suffix), pstreamp);
> +    return new_fd_pstream(name, fd, punix_accept, NULL, bind_path, pstreamp);
>  }
>  
>  static int
> diff --git a/tests/jsonrpc.at b/tests/jsonrpc.at
> index 2a7f91b..664debe 100644
> --- a/tests/jsonrpc.at
> +++ b/tests/jsonrpc.at
> @@ -1,6 +1,7 @@
>  AT_BANNER([JSON-RPC - C])
>  
>  AT_SETUP([JSON-RPC request and successful reply])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  AT_CHECK([test-jsonrpc --detach --no-chdir --pidfile="`pwd`"/pid listen punix:socket])
>  AT_CHECK([test -s pid])
>  AT_CHECK([kill -0 `cat pid`])
> @@ -12,6 +13,7 @@ AT_CHECK([kill `cat pid`])
>  AT_CLEANUP
>  
>  AT_SETUP([JSON-RPC request and error reply])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  AT_CHECK([test-jsonrpc --detach --no-chdir --pidfile="`pwd`"/pid listen punix:socket])
>  AT_CHECK([test -s pid])
>  AT_CHECK([kill -0 `cat pid`])
> @@ -23,6 +25,7 @@ AT_CHECK([kill `cat pid`])
>  AT_CLEANUP
>  
>  AT_SETUP([JSON-RPC notification])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  AT_CHECK([test-jsonrpc --detach --no-chdir --pidfile="`pwd`"/pid listen punix:socket])
>  AT_CHECK([test -s pid])
>  # When a daemon dies it deletes its pidfile, so make a copy.
> diff --git a/tests/ovsdb-execution.at b/tests/ovsdb-execution.at
> index 6a3b5d1..eec2a04 100644
> --- a/tests/ovsdb-execution.at
> +++ b/tests/ovsdb-execution.at
> @@ -138,6 +138,7 @@ m4_divert_pop([PREPARE_TESTS])
>  m4_define([OVSDB_CHECK_EXECUTION],
>    [AT_SETUP([$1])
>     AT_KEYWORDS([ovsdb execute execution positive $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     AT_CHECK([test-ovsdb execute "`$2`" m4_foreach([txn], [$3], [ 'txn'])],
>       [0], [stdout], [])
>     AT_CHECK([perl $srcdir/uuidfilt.pl stdout], [0], [$4])
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index ce22010..a206f62 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -19,6 +19,7 @@ AT_BANNER([OVSDB -- interface description language (IDL)])
>  m4_define([OVSDB_CHECK_IDL_C],
>    [AT_SETUP([$1 - C])
>     AT_KEYWORDS([ovsdb server idl positive $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     AT_CHECK([ovsdb-tool create db $abs_srcdir/idltest.ovsschema],
>                    [0], [stdout], [ignore])
>     AT_CHECK([ovsdb-server '-vPATTERN:console:ovsdb-server|%c|%m' --detach --no-chdir --pidfile="`pwd`"/pid --remote=punix:socket --unixctl="`pwd`"/unixctl db], [0], [ignore], [ignore])
> @@ -36,6 +37,7 @@ m4_define([OVSDB_CHECK_IDL_PY],
>    [AT_SETUP([$1 - Python])
>     AT_SKIP_IF([test $HAVE_PYTHON = no])
>     AT_KEYWORDS([ovsdb server idl positive Python $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     AT_CHECK([ovsdb-tool create db $abs_srcdir/idltest.ovsschema],
>                    [0], [stdout], [ignore])
>     AT_CHECK([ovsdb-server '-vPATTERN:console:ovsdb-server|%c|%m' --detach --no-chdir --pidfile="`pwd`"/pid --remote=punix:socket --unixctl="`pwd`"/unixctl db], [0], [ignore], [ignore])
> @@ -53,6 +55,7 @@ m4_define([OVSDB_CHECK_IDL_TCP_PY],
>    [AT_SETUP([$1 - Python tcp])
>     AT_SKIP_IF([test $HAVE_PYTHON = no])
>     AT_KEYWORDS([ovsdb server idl positive Python with tcp socket $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     AT_CHECK([ovsdb-tool create db $abs_srcdir/idltest.ovsschema],
>                    [0], [stdout], [ignore])
>     AT_CHECK([perl $srcdir/choose-port.pl], [0], [stdout])
> diff --git a/tests/ovsdb-macros.at b/tests/ovsdb-macros.at
> index c1aa619..2aa752b 100644
> --- a/tests/ovsdb-macros.at
> +++ b/tests/ovsdb-macros.at
> @@ -2,7 +2,8 @@ dnl OVSDB_INIT([$1])
>  dnl
>  dnl Creates an empty database named $1.
>  m4_define([OVSDB_INIT],
> -  [AT_CHECK(
> +  [OVS_RUNDIR=`pwd`; export OVS_RUNDIR
> +   AT_CHECK(
>       [ovsdb-tool create $1 $abs_top_srcdir/vswitchd/vswitch.ovsschema],
>       [0], [stdout], [ignore])
>     AT_CHECK(
> diff --git a/tests/ovsdb-monitor.at b/tests/ovsdb-monitor.at
> index 167b44c..aff5854 100644
> --- a/tests/ovsdb-monitor.at
> +++ b/tests/ovsdb-monitor.at
> @@ -19,6 +19,7 @@ AT_BANNER([OVSDB -- ovsdb-server monitors])
>  m4_define([OVSDB_CHECK_MONITOR], 
>    [AT_SETUP([$1])
>     AT_KEYWORDS([ovsdb server monitor positive $9])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     $2 > schema
>     AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
>     m4_foreach([txn], [$3],
> diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
> index 6dcf2f5..c5feec9 100644
> --- a/tests/ovsdb-server.at
> +++ b/tests/ovsdb-server.at
> @@ -22,6 +22,7 @@ m4_define([OVSDB_SERVER_SHUTDOWN],
>  m4_define([OVSDB_CHECK_EXECUTION], 
>    [AT_SETUP([$1])
>     AT_KEYWORDS([ovsdb server positive unix $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     $2 > schema
>     AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
>     AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/pid --remote=punix:socket --unixctl="`pwd`"/unixctl db], [0], [ignore], [ignore])
> @@ -39,6 +40,7 @@ EXECUTION_EXAMPLES
>  
>  AT_SETUP([truncating corrupted database log])
>  AT_KEYWORDS([ovsdb server positive unix])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  ordinal_schema > schema
>  AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
>  dnl Do one transaction and save the output.
> @@ -85,6 +87,7 @@ AT_CLEANUP
>  
>  AT_SETUP([truncating database log with bad transaction])
>  AT_KEYWORDS([ovsdb server positive unix])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  ordinal_schema > schema
>  AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
>  dnl Do one transaction and save the output.
> @@ -132,6 +135,7 @@ AT_CLEANUP
>  
>  AT_SETUP([ovsdb-client get-schema-version])
>  AT_KEYWORDS([ovsdb server positive])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  ordinal_schema > schema
>  AT_CHECK([ovsdb-tool create db schema], [0], [ignore], [ignore])
>  AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/pid --unixctl="`pwd`"/unixctl --remote=punix:socket db], [0], [ignore], [ignore])
> @@ -142,6 +146,7 @@ AT_CLEANUP
>  
>  AT_SETUP([database multiplexing implementation])
>  AT_KEYWORDS([ovsdb server positive])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  ordinal_schema > schema1
>  constraint_schema > schema2
>  AT_CHECK([ovsdb-tool create db1 schema1], [0], [ignore], [ignore])
> @@ -280,6 +285,7 @@ AT_CLEANUP
>  
>  AT_SETUP([compacting online])
>  AT_KEYWORDS([ovsdb server compact])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  ordinal_schema > schema
>  dnl Make sure that "ovsdb-tool create" works with a dangling symlink for
>  dnl the database and the lockfile, creating the target of each symlink rather
> @@ -430,6 +436,7 @@ m4_define([OVSDB_CHECK_EXECUTION],
>    [AT_SETUP([$1])
>     AT_KEYWORDS([ovsdb server positive ssl $5])
>     AT_SKIP_IF([test "$HAVE_OPENSSL" = no])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     $2 > schema
>     AT_CHECK([perl $srcdir/choose-port.pl], [0], [stdout])
>     SSL_PORT=`cat stdout`
> @@ -479,6 +486,7 @@ AT_CLEANUP])
>  m4_define([OVSDB_CHECK_EXECUTION],
>    [AT_SETUP([$1])
>     AT_KEYWORDS([ovsdb server positive tcp $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     $2 > schema
>     AT_CHECK([perl $srcdir/choose-port.pl], [0], [stdout])
>     TCP_PORT=`cat stdout`
> @@ -519,6 +527,7 @@ AT_BANNER([OVSDB -- transactions on transient ovsdb-server])
>  m4_define([OVSDB_CHECK_EXECUTION], 
>    [AT_SETUP([$1])
>     AT_KEYWORDS([ovsdb server positive transient $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     $2 > schema
>     AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
>     m4_foreach([txn], [$3], 
> diff --git a/tests/ovsdb-tool.at b/tests/ovsdb-tool.at
> index e4f4a29..286d700 100644
> --- a/tests/ovsdb-tool.at
> +++ b/tests/ovsdb-tool.at
> @@ -16,6 +16,7 @@ AT_BANNER([OVSDB -- ovsdb-tool])
>  m4_define([OVSDB_CHECK_EXECUTION], 
>    [AT_SETUP([$1])
>     AT_KEYWORDS([ovsdb file positive $5])
> +   OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>     $2 > schema
>     touch .db.~lock~
>     AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
> @@ -48,6 +49,7 @@ AT_CLEANUP
>  
>  AT_SETUP([ovsdb-tool compact])
>  AT_KEYWORDS([ovsdb file positive])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  ordinal_schema > schema
>  dnl Make sure that "ovsdb-tool create" works with a dangling symlink,
>  dnl creating the target of the symlink rather than replacing the symlink
> @@ -155,6 +157,7 @@ AT_CLEANUP
>  
>  AT_SETUP([ovsdb-tool convert -- removing a column])
>  AT_KEYWORDS([ovsdb file positive])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  ordinal_schema > schema
>  AT_DATA([new-schema], 
>    [[{"name": "ordinals",
> @@ -218,6 +221,7 @@ AT_CLEANUP
>  
>  AT_SETUP([ovsdb-tool convert -- adding a column])
>  AT_KEYWORDS([ovsdb file positive])
> +OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>  AT_DATA([schema], 
>    [[{"name": "ordinals",
>       "tables": {
> diff --git a/tests/vconn.at b/tests/vconn.at
> index ae095b0..67a4224 100644
> --- a/tests/vconn.at
> +++ b/tests/vconn.at
> @@ -11,6 +11,7 @@ m4_define([TEST_VCONN_CLASS],
>        [send-short-hello],
>        [send-invalid-version-hello]],
>       [AT_SETUP([$1 vconn - m4_bpatsubst(testname, [-], [ ])])
> +      OVS_RUNDIR=`pwd`; export OVS_RUNDIR
>        m4_if([$1], [ssl], [
>          AT_SKIP_IF([test "$HAVE_OPENSSL" = no])
>          AT_CHECK([cp $abs_top_builddir/tests/testpki*.pem .])])
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index f5a4366..fdd7c64 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2799,8 +2799,10 @@ bridge_configure_remotes(struct bridge *br,
>              if (!strncmp(c->target, "unix:", 5)) {
>                  /* Connect to a listening socket */
>                  whitelist = xasprintf("unix:%s/", ovs_rundir());
> -                if (!equal_pathnames(c->target, whitelist,
> -                                     strlen(whitelist))) {
> +                if (strchr(c->target, '/') &&
> +                   !equal_pathnames(c->target, whitelist,
> +                     strlen(whitelist))) {
> +                    /* Absolute path specified, but not in ovs_rundir */
>                      VLOG_ERR_RL(&rl, "bridge %s: Not connecting to socket "
>                                    "controller \"%s\" due to possibility for "
>                                    "remote exploit.  Instead, specify socket "
> -- 
> 1.7.10.4



More information about the dev mailing list