On Mon, Apr 18, 2011 at 01:08:43PM +0900, YAMAMOTO Takashi wrote:
> >> --- a/lib/socket-util.c
> >> +++ b/lib/socket-util.c
> >> @@ -349,7 +349,7 @@ make_unix_socket(int style, bool nonblock, bool
> >> passcred OVS_UNUSED,
> >>
> >> error = make_sockaddr_un(bind_path, &un, &un_len, &dirfd);
> >> if (!error && (bind(fd, (struct sockaddr*) &un, un_len)
> >> - || fchmod(fd, S_IRWXU))) {
> >> + || chmod(bind_path, S_IRWXU))) {
> >> error = errno;
> >> }
> >> if (dirfd >= 0) {
> >
> > That change introduces a race.
> >
> > Here's a fix that I have tested to work. What do you think?
>
> is this piece of code intended to be linux-only?
> even if so, i don't think relying on this rather undocumented behaviour
> is a good idea.
>
> i think a common way to solve this kind of race is to create a directory
> with a desired permission and then bind in the directory.
>
> (i don't know if this race can really be a problem because i'm not
> familiar with this code.)
This code is not intended to be Linux-only, but Linux is the most
important target.
It is better to avoid the race, but the race is not important in the
most common case because the socket is being created in /var/run or
/var/run/openvswitch, which are ordinarily writable only by root.
Creating a directory isn't a good solution here, because the socket in
some cases is supposed to have a user-specifiable name. I guess that
we could create a directory, bind() in the directory, rename() the
file out of the directory, and then remove the directory, but that
leaves a lot of cases for cleanup in the case of signals that arrive
in the middle of the operation.
I took a look at _Unix Network Programming_ and saw that Stevens says
that the umask is supposed to affect the permissions of a file created
by bind(). So setting the umask should be fairly portable.
With all that taken into account, here is a new version. On Linux, I
prefer the fchmod() based solution, because someday OVS may have to
add support for threads and the umask is not thread-local.
What do you think of this version?
Thanks,
Ben.
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <[email protected]>
Date: Mon, 18 Apr 2011 11:24:50 -0700
Subject: [PATCH] socket-util: Properly set socket permissions in
make_unix_socket().
Under Linux, at least, bind and fchmod interact for Unix sockets in a way
that surprised me. Calling fchmod() on a Unix socket successfully sets the
permissions for the socket's own inode. But that has no effect on any
inode that has already been created in the file system by bind(), because
that inode is not the same as the one for the Unix socket itself.
However, if you bind() *after* calling fchmod(), then the bind() takes the
permissions for the new inode from the Unix socket inode, which has the
desired effect.
This also adds a more portable fallback for non-Linux systems.
Reported-by: YAMAMOTO Takashi <[email protected]>
---
lib/socket-util.c | 23 ++++++++++++++++++++---
1 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/lib/socket-util.c b/lib/socket-util.c
index 12bbc71..7e4b8be 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -302,6 +302,24 @@ make_sockaddr_un(const char *name, struct sockaddr_un *un,
socklen_t *un_len,
}
}
+/* Binds Unix domain socket 'fd' to a file with permissions 0700. */
+static int
+bind_unix_socket(int fd, struct sockaddr *sun, socklen_t sun_len)
+{
+#ifdef __linux__
+ /* On Linux, calling fchmod() *before* bind() sets permissions for the file
+ * about to be created. Calling fchmod() *after* bind has no effect on the
+ * file that was created.) */
+ return fchmod(fd, 0700) || bind(fd, sun, sun_len) ? errno : 0;
+#else
+ /* 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;
+#endif
+}
+
/* Creates a Unix domain socket in the given 'style' (either SOCK_DGRAM or
* SOCK_STREAM) that is bound to '*bind_path' (if 'bind_path' is non-null) and
* connected to '*connect_path' (if 'connect_path' is non-null). If 'nonblock'
@@ -348,9 +366,8 @@ make_unix_socket(int style, bool nonblock, bool passcred
OVS_UNUSED,
fatal_signal_add_file_to_unlink(bind_path);
error = make_sockaddr_un(bind_path, &un, &un_len, &dirfd);
- if (!error && (bind(fd, (struct sockaddr*) &un, un_len)
- || fchmod(fd, S_IRWXU))) {
- error = errno;
+ if (!error) {
+ error = bind_unix_socket(fd, (struct sockaddr *) &un, un_len);
}
if (dirfd >= 0) {
close(dirfd);
--
1.7.1
_______________________________________________
discuss mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/discuss
