From owner-freebsd-bugs@FreeBSD.ORG Mon Feb 4 02:10:01 2013 Return-Path: Delivered-To: freebsd-bugs@smarthost.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 74BB04B9 for ; Mon, 4 Feb 2013 02:10:01 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) by mx1.freebsd.org (Postfix) with ESMTP id 448B8729 for ; Mon, 4 Feb 2013 02:10:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.6/8.14.6) with ESMTP id r142A1nD023256 for ; Mon, 4 Feb 2013 02:10:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.6/8.14.6/Submit) id r142A1NR023255; Mon, 4 Feb 2013 02:10:01 GMT (envelope-from gnats) Date: Mon, 4 Feb 2013 02:10:01 GMT Message-Id: <201302040210.r142A1NR023255@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org Cc: From: David Xu Subject: Re: kern/175674: sem_open() should use O_EXLOCK with open() instead of a separate flock() call X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: David Xu List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Feb 2013 02:10:01 -0000 The following reply was made to PR kern/175674; it has been noted by GNATS. From: David Xu To: Jilles Tjoelker Cc: Giorgos Keramidas , Jukka Ukkonen , freebsd-gnats-submit@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 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. > Note that EINTR is allowed to be returned for sem_open(). http://pubs.opengroup.org/onlinepubs/009604499/functions/sem_open.html Regards, David Xu