svn commit: r244735 - head/lib/libutil

[Jilles Tjoelker]

On Thu, Dec 27, 2012 at 02:09:50PM +0000, Baptiste Daroussin wrote:
> Author: bapt
> Date: Thu Dec 27 14:09:50 2012
> New Revision: 244735
> URL: http://svnweb.freebsd.org/changeset/base/244735

> Log:
>   Use flopen(3) instead of open(2) + flock(2)

> Modified:
>   head/lib/libutil/gr_util.c
>   head/lib/libutil/pw_util.c

> Modified: head/lib/libutil/gr_util.c
> ==============================================================================
> --- head/lib/libutil/gr_util.c	Thu Dec 27 13:21:37 2012	(r244734)
> +++ head/lib/libutil/gr_util.c	Thu Dec 27 14:09:50 2012	(r244735)
> @@ -106,10 +106,8 @@ gr_lock(void)
>  	for (;;) {
>  		struct stat st;
>  
> -		lockfd = open(group_file, O_RDONLY, 0);
> -		if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1)
> -			err(1, "%s", group_file);
> -		if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) {
> +		lockfd = flopen(group_file, O_RDONLY|O_NONBLOCK, 0);
> +		if (lockfd == -1) {
>  			if (errno == EWOULDBLOCK) {
>  				errx(1, "the group file is busy");
>  			} else {
> 
> Modified: head/lib/libutil/pw_util.c
> ==============================================================================
> --- head/lib/libutil/pw_util.c	Thu Dec 27 13:21:37 2012	(r244734)
> +++ head/lib/libutil/pw_util.c	Thu Dec 27 14:09:50 2012	(r244735)
> @@ -179,11 +179,8 @@ pw_lock(void)
>  	for (;;) {
>  		struct stat st;
>  
> -		lockfd = open(masterpasswd, O_RDONLY, 0);
> -		if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1)
> -			err(1, "%s", masterpasswd);
> -		/* XXX vulnerable to race conditions */
> -		if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) {
> +		lockfd = flopen(masterpasswd, O_RDONLY|O_NONBLOCK, 0);
> +		if (lockfd == -1) {
>  			if (errno == EWOULDBLOCK) {
>  				errx(1, "the password db file is busy");
>  			} else {

The fcntl(lockfd, F_SETFD, 1) was removed and not replaced by anything.
The comment in pw_util.c gives the impression that this is vital for
security; in fact, chpass does not keep the password file locked across
a user's editor. However, the idea of setting close-on-exec on this file
descriptor is good. Please add |O_CLOEXEC to the flopen() flags in both
cases.

-- 
Jilles Tjoelker