kern/175674: sem_open() should use O_EXLOCK with open() instead of a separate flock() call

[David Xu]

The following reply was made to PR kern/175674; it has been noted by GNATS.

From: David Xu <davidxu at freebsd.org>
To: Jilles Tjoelker <jilles at stack.nl>
Cc: Giorgos Keramidas <keramida at FreeBSD.org>, Jukka Ukkonen <jau at iki.fi>,
        freebsd-gnats-submit at FreeBSD.org
Subject: Re: kern/175674: sem_open() should use O_EXLOCK with open() instead
 of a separate flock() call
Date: Mon, 04 Feb 2013 10:08:54 +0800

 On 2013/02/04 04:20, Jilles Tjoelker wrote:
 > On Sun, Feb 03, 2013 at 06:25:25AM +0100, Giorgos Keramidas wrote:
 >> On 2013-01-29 18:03, Jukka Ukkonen <jau at 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 at 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.
 >
 Note that EINTR is allowed to be returned for sem_open().
 http://pubs.opengroup.org/onlinepubs/009604499/functions/sem_open.html
 
 
 Regards,
 David Xu