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 <f...@redhat.com>
>
> CC: YAMAMOTO Takashi <yamam...@valinux.co.jp>
> Signed-off-by: Ben Pfaff <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
