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: Giorgos Keramidas <keramida at FreeBSD.org>
Cc: Jukka Ukkonen <jau at iki.fi>, freebsd-gnats-submit at FreeBSD.org,
        jilles 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:05:31 +0800

 On 2013/02/03 13:25, 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;
 >
 >
 I don't think there is a race condition, the flock is used to ensure
 the semaphore is already initialized, whether you use flock or E_EXLOCK,
 they are same, because only one thread can lock the file at same time,
 and in locked state, the code checks and initializes semaphore if it is
 needed.
 
 Regards,
 David Xu