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
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to