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
_______________________________________________
discuss mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/discuss
