[ovs-discuss] suspicious usage of fchmod

YAMAMOTO Takashi yamamoto at valinux.co.jp
Tue Apr 19 00:30:54 UTC 2011


hi,

> 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?

it looks like a reasonable compromise to me.  thanks.

YAMAMOTO Takashi



More information about the discuss mailing list