[ovs-dev] [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.

Ben Pfaff blp at nicira.com
Thu Feb 7 15:39:15 UTC 2013


On Wed, Feb 06, 2013 at 11:28:55AM -0800, Pavithra Ramesh wrote:
> Taken care of the memroy leak, used xasprintf instead.
> Also included the change in bridge.c to relax the whitelist
> check.
> 
> Signed-off-by: Pavithra Ramesh <paramesh at vmware.com>
> ---
>  lib/stream-unix.c |   11 ++++++++++-
>  vswitchd/bridge.c |    6 ++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/stream-unix.c b/lib/stream-unix.c
> index 6ed7648..a71108c 100644
> --- a/lib/stream-unix.c
> +++ b/lib/stream-unix.c
> @@ -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,23 @@ static int
>  unix_open(const char *name, char *suffix, struct stream **streamp,
>            uint8_t dscp OVS_UNUSED)
>  {
> -    const char *connect_path = suffix;
> +    const char *ovsDir = ovs_rundir();

The OVS coding style uses identifiers like_this not likeThis.

ovsDir is only used inside the if statement below so I would move the
declaration in there.

> +    char *new_path = NULL, *connect_path = suffix;

If you want to declare variables with initializers then declare each one
on a separate line please.

>      int fd;
>  
> +    if (suffix[0] != '/') {
> +        /* Not specified absolute path */

We usually write code comments as sentences.

> +        new_path = xasprintf("%s/%s", ovsDir, suffix);
> +        connect_path = new_path;
> +    }
>      fd = make_unix_socket(SOCK_STREAM, true, NULL, connect_path);
>      if (fd < 0) {
>          VLOG_DBG("%s: connection failed (%s)", connect_path, strerror(-fd));
> +        free(new_path);        
>          return -fd;
>      }
>  
> +    free(new_path);        
>      return new_fd_stream(name, fd, check_connection_completion(fd), streamp);
>  }
>  
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index f5a4366..ed51ab4 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 ((c->target[5] == '/') &&
> +                   (!equal_pathnames(c->target, whitelist,
> +                     strlen(whitelist)))) {

Please don't add the extra unnecessary () around each clause in the if
statement here.

By the way, I don't think this is secure, because a relative path could
also access outside ovs_rundir() if it contained "..".

> +                    /* 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 "

Thanks,

Ben.



More information about the dev mailing list