Date: Sun, 3 Feb 2013 20:30:01 GMT From: Jilles Tjoelker <jilles@stack.nl> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/175674: sem_open() should use O_EXLOCK with open() instead of a separate flock() call Message-ID: <201302032030.r13KU1Yo039578@freefall.freebsd.org>
index | next in thread | raw e-mail
The following reply was made to PR kern/175674; it has been noted by GNATS.
From: Jilles Tjoelker <jilles@stack.nl>
To: Giorgos Keramidas <keramida@FreeBSD.org>
Cc: Jukka Ukkonen <jau@iki.fi>, freebsd-gnats-submit@FreeBSD.org,
davidxu@FreeBSD.org
Subject: Re: kern/175674: sem_open() should use O_EXLOCK with open() instead
of a separate flock() call
Date: Sun, 3 Feb 2013 21:20:31 +0100
On Sun, Feb 03, 2013 at 06:25:25AM +0100, Giorgos Keramidas wrote:
> On 2013-01-29 18:03, Jukka Ukkonen <jau@iki.fi> wrote:
> > >Number: 175674
> > >Category: kern
> > >Synopsis: sem_open() should use O_EXLOCK with open() instead of a separate flock() call
>
> > >Environment:
> > FreeBSD sleipnir 9.1-STABLE FreeBSD 9.1-STABLE #2 r246056M: Tue Jan 29 07:33:01 EET 2013 root@sleipnir:/usr/obj/usr/src/sys/Sleipnir amd64
> > >Description:
> > sem_open() is calling flock() to set a lock on a newly created file descriptor.
> > That is pointless. The open() call a few lines before the flock() could, and
> > in my opinion should, be done with the O_EXLOCK flag set.
> It's also a bit safer to obtain the exclusive lock atomically before
> open() returns. Waiting for open() to complete and then calling flock()
> has a race condition.
> Jilles and David, do you think this patch looks ok for libc?
> > Patch attached with submission follows:
> >
> > --- lib/libc/gen/sem_new.c.flock 2012-11-09 18:50:05.000000000 +0200
> > +++ lib/libc/gen/sem_new.c 2012-11-09 18:44:59.000000000 +0200
> > @@ -198,11 +198,13 @@
> > goto error;
> > }
> >
> > - fd = _open(path, flags|O_RDWR|O_CLOEXEC, mode);
> > + fd = _open(path, flags|O_RDWR|O_CLOEXEC|O_EXLOCK, mode);
> > if (fd == -1)
> > goto error;
> > +#if 0
> > if (flock(fd, LOCK_EX) == -1)
> > goto error;
> > +#endif
> > if (_fstat(fd, &sb)) {
> > flock(fd, LOCK_UN);
> > goto error;
For a reason unknown to me, open(2) does not restart but always returns
[EINTR] when a signal is caught. This is not POSIX-compliant. On the
other hand, flock(2) is not broken in this way. So this change breaks
sem_open(3) in the unlikely case that a signal with SA_RESTART arrives
while it is waiting for the lock.
The best way to fix this is in kern_openat() in the kernel but this
might cause compatibility issues.
The #if 0 is silly; we have version control to restore old code if need
be.
--
Jilles Tjoelker
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201302032030.r13KU1Yo039578>