[ovs-dev] [PATCH] socket-util-unix: Fix umask race in bind_unix_socket().

Flavio Leitner fbl at redhat.com
Thu Aug 7 17:31:24 UTC 2014


On Thu, Jul 24, 2014 at 12:53:30PM -0700, Ben Pfaff wrote:
> The umask is a process-wide value, so bind_unix_socket() races with file
> creation in other Open vSwitch threads.  This fixes the race.
> 
> The workaround for non-Linux systems is not ideal, but I do not know any
> other general solution.  I tested the workaround only on Linux.

The Linux part looks good. Although my style preference for
readability is like:

if (LINUX) {
   /* On Linux, the fd's permissions become the file's permissions.
      fchmod() does not affect other files, like umask() does. */
   if (fchmod(fd, mode)) {
       return errno;
   }

   /* must be after fchmod */
   if (bind(fd, sun, sun_len))
       return errno;
   }

   return 0;
}

I can't tell about the specifics of FreeBSD and NetBSD though
the work around looks sane too.

Acked-by: Flavio Leitner <fbl at redhat.com>

> 
> CC: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/socket-util-unix.c |   35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/socket-util-unix.c b/lib/socket-util-unix.c
> index 6b451d4..59b8160 100644
> --- a/lib/socket-util-unix.c
> +++ b/lib/socket-util-unix.c
> @@ -23,6 +23,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/un.h>
> +#include <sys/wait.h>
>  #include <unistd.h>
>  #include "fatal-signal.h"
>  #include "random.h"
> @@ -261,11 +262,35 @@ free_sockaddr_un(int dirfd, const char *linkname)
>  static int
>  bind_unix_socket(int fd, struct sockaddr *sun, socklen_t sun_len)
>  {
> -    /* According to _Unix Network Programming_, umask should affect bind(). */
> -    mode_t old_umask = umask(0077);
> -    int error = bind(fd, sun, sun_len) ? errno : 0;
> -    umask(old_umask);
> -    return error;
> +    const mode_t mode = 0700;
> +    if (LINUX) {
> +        /* On Linux, the fd's permissions become the file's permissions.  */
> +        return fchmod(fd, mode) || bind(fd, sun, sun_len) ? errno : 0;
> +    } else {
> +        /* On FreeBSD and NetBSD, only the umask affects permissions.  The
> +         * umask is process-wide rather than thread-specific, so we have to use
> +         * a subprocess for safety. */
> +        pid_t pid = fork();
> +
> +        if (!pid) {
> +            umask(mode ^ 0777);
> +            _exit(bind(fd, sun, sun_len) ? errno : 0);
> +        } else if (pid > 0) {
> +            int status;
> +            int error;
> +
> +            do {
> +                error = waitpid(pid, &status, 0) < 0 ? errno : 0;
> +            } while (error == EINTR);
> +
> +            return (error ? error
> +                    : WIFEXITED(status) ? WEXITSTATUS(status)
> +                    : WIFSIGNALED(status) ? EINTR
> +                    : ECHILD /* WTF? */);
> +        } else {
> +            return errno;
> +        }
> +    }
>  }
>  
>  /* Creates a Unix domain socket in the given 'style' (either SOCK_DGRAM or
> -- 
> 1.7.10.4
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
> 



More information about the dev mailing list