svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb

Ian Lepore ian at freebsd.org
Thu Jul 2 18:20:35 UTC 2015


On Thu, 2015-07-02 at 20:00 +0200, Ermal Luçi wrote:
> On Thu, Jul 2, 2015 at 7:31 PM, Renato Botelho <garga at freebsd.org> wrote:
> 
> > Author: garga (ports committer)
> > Date: Thu Jul  2 17:30:59 2015
> > New Revision: 285050
> > URL: https://svnweb.freebsd.org/changeset/base/285050
> >
> > Log:
> >   When passwd or group information is changed (by pw, vipw, chpass, ...)
> >   temporary file is created and then a rename() call move it to official
> > file.
> >   This operation didn't have any check to make sure data was written to
> > disk
> >   and if a power cycle happens system could end up with a 0 length passwd
> >   or group database.
> >
> >   There is a pfSense bug with more infor about it:
> >
> >   https://redmine.pfsense.org/issues/4523
> >
> >   The following changes were made to protect passwd and group operations:
> >
> >   * lib/libutil/gr_util.c:
> >    - Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
> >    - After rename(), fsync() call on directory for faster result
> >
> >   * lib/libutil/pw_util.c
> >    - Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
> >
> >   * usr.sbin/pwd_mkdb/pwd_mkdb.c
> >    - Added O_SYNC flag on dbopen() calls
> >    - After rename(), fsync() call on directory for faster result
> >
> >   * lib/libutil/pw_util.3
> >    - pw_lock() returns a file descriptor to master password file on success
> >
> >   Differential Revision:        https://reviews.freebsd.org/D2978
> >   Approved by:  bapt
> >   Sponsored by: Netgate
> >
> > Modified:
> >   head/lib/libutil/gr_util.c
> >   head/lib/libutil/pw_util.3
> >   head/lib/libutil/pw_util.c
> >   head/usr.sbin/pwd_mkdb/pwd_mkdb.c
> >
> > Modified: head/lib/libutil/gr_util.c
> >
> > ==============================================================================
> > --- head/lib/libutil/gr_util.c  Thu Jul  2 16:17:05 2015        (r285049)
> > +++ head/lib/libutil/gr_util.c  Thu Jul  2 17:30:59 2015        (r285050)
> > @@ -141,7 +141,7 @@ gr_tmp(int mfd)
> >                 errno = ENAMETOOLONG;
> >                 return (-1);
> >         }
> > -       if ((tfd = mkstemp(tempname)) == -1)
> > +       if ((tfd = mkostemp(tempname, O_SYNC)) == -1)
> >                 return (-1);
> >         if (mfd != -1) {
> >                 while ((nr = read(mfd, buf, sizeof(buf))) > 0)
> > @@ -318,10 +318,28 @@ gr_copy(int ffd, int tfd, const struct g
> >  int
> >  gr_mkdb(void)
> >  {
> > +       int fd;
> > +
> >         if (chmod(tempname, 0644) != 0)
> >                 return (-1);
> >
> > -       return (rename(tempname, group_file));
> > +       if (rename(tempname, group_file) != 0)
> > +               return (-1);
> > +
> > +       /*
> > +        * Make sure new group file is safe on disk. To improve
> > performance we
> > +        * will call fsync() to the directory where file lies
> > +        */
> > +       if ((fd = open(group_dir, O_RDONLY|O_DIRECTORY)) == -1)
> > +               return (-1);
> > +
> >
> 
> This really is not a real failure!
> Not sure how you would report this but it really is not a failure since the
> rename has completed and you are giving false information back.
> 

I disagree.  If you can't open the directory containing the file you
just renamed into that directory, then the assertion that the rename
completed strikes me as speculative at best.  IMO, it's not truly
completed until the fsync() returns success.

-- Ian

> 
> 
> > +       if (fsync(fd) != 0) {
> > +               close(fd);
> > +               return (-1);
> > +       }
> > +
> > +       close(fd);
> > +       return(0);
> >  }
> >
> >  /*
> >
> > Modified: head/lib/libutil/pw_util.3
> >
> > ==============================================================================
> > --- head/lib/libutil/pw_util.3  Thu Jul  2 16:17:05 2015        (r285049)
> > +++ head/lib/libutil/pw_util.3  Thu Jul  2 17:30:59 2015        (r285050)
> > @@ -233,7 +233,8 @@ function returns 0 in case of success an
> >  The
> >  .Fn pw_lock
> >  function locks the master password file.
> > -It returns 0 in case of success and -1 in case of failure.
> > +It returns a file descriptor to master password file in case of success
> > +and -1 in case of failure.
> >  .Pp
> >  The
> >  .Fn pw_scan
> >
> > Modified: head/lib/libutil/pw_util.c
> >
> > ==============================================================================
> > --- head/lib/libutil/pw_util.c  Thu Jul  2 16:17:05 2015        (r285049)
> > +++ head/lib/libutil/pw_util.c  Thu Jul  2 17:30:59 2015        (r285050)
> > @@ -226,7 +226,7 @@ pw_tmp(int mfd)
> >                 errno = ENAMETOOLONG;
> >                 return (-1);
> >         }
> > -       if ((tfd = mkstemp(tempname)) == -1)
> > +       if ((tfd = mkostemp(tempname, O_SYNC)) == -1)
> >                 return (-1);
> >         if (mfd != -1) {
> >                 while ((nr = read(mfd, buf, sizeof(buf))) > 0)
> >
> > Modified: head/usr.sbin/pwd_mkdb/pwd_mkdb.c
> >
> > ==============================================================================
> > --- head/usr.sbin/pwd_mkdb/pwd_mkdb.c   Thu Jul  2 16:17:05 2015
> > (r285049)
> > +++ head/usr.sbin/pwd_mkdb/pwd_mkdb.c   Thu Jul  2 17:30:59 2015
> > (r285050)
> > @@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$");
> >  #include <err.h>
> >  #include <errno.h>
> >  #include <fcntl.h>
> > +#include <libgen.h>
> >  #include <limits.h>
> >  #include <pwd.h>
> >  #include <signal.h>
> > @@ -227,14 +228,14 @@ main(int argc, char *argv[])
> >                 clean = FILE_INSECURE;
> >                 cp(buf2, buf, PERM_INSECURE);
> >                 dp = dbopen(buf,
> > -                   O_RDWR|O_EXCL, PERM_INSECURE, DB_HASH, &openinfo);
> > +                   O_RDWR|O_EXCL|O_SYNC, PERM_INSECURE, DB_HASH,
> > &openinfo);
> >                 if (dp == NULL)
> >                         error(buf);
> >
> >                 clean = FILE_SECURE;
> >                 cp(sbuf2, sbuf, PERM_SECURE);
> >                 sdp = dbopen(sbuf,
> > -                   O_RDWR|O_EXCL, PERM_SECURE, DB_HASH, &openinfo);
> > +                   O_RDWR|O_EXCL|O_SYNC, PERM_SECURE, DB_HASH, &openinfo);
> >                 if (sdp == NULL)
> >                         error(sbuf);
> >
> > @@ -291,13 +292,13 @@ main(int argc, char *argv[])
> >                 method = 0;
> >         } else {
> >                 dp = dbopen(buf,
> > -                   O_RDWR|O_CREAT|O_EXCL, PERM_INSECURE, DB_HASH,
> > &openinfo);
> > +                   O_RDWR|O_CREAT|O_EXCL|O_SYNC, PERM_INSECURE, DB_HASH,
> > &openinfo);
> >                 if (dp == NULL)
> >                         error(buf);
> >                 clean = FILE_INSECURE;
> >
> >                 sdp = dbopen(sbuf,
> > -                   O_RDWR|O_CREAT|O_EXCL, PERM_SECURE, DB_HASH,
> > &openinfo);
> > +                   O_RDWR|O_CREAT|O_EXCL|O_SYNC, PERM_SECURE, DB_HASH,
> > &openinfo);
> >                 if (sdp == NULL)
> >                         error(sbuf);
> >                 clean = FILE_SECURE;
> > @@ -721,13 +722,27 @@ void
> >  mv(char *from, char *to)
> >  {
> >         char buf[MAXPATHLEN];
> > +       char *to_dir;
> > +       int to_dir_fd = -1;
> >
> > -       if (rename(from, to)) {
> > +       /*
> > +        * Make sure file is safe on disk. To improve performance we will
> > call
> > +        * fsync() to the directory where file lies
> > +        */
> > +       if (rename(from, to) != 0 ||
> > +           (to_dir = dirname(to)) == NULL ||
> > +           (to_dir_fd = open(to_dir, O_RDONLY|O_DIRECTORY)) == -1 ||
> > +           fsync(to_dir_fd) != 0) {
> >                 int sverrno = errno;
> >                 (void)snprintf(buf, sizeof(buf), "%s to %s", from, to);
> >                 errno = sverrno;
> > +               if (to_dir_fd != -1)
> > +                       close(to_dir_fd);
> >                 error(buf);
> >         }
> > +
> > +       if (to_dir_fd != -1)
> > +               close(to_dir_fd);
> >  }
> >
> >  void
> >
> > --
> > Ermal
> >




More information about the svn-src-head mailing list