svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb
Mateusz Guzik
mjguzik at gmail.com
Thu Jul 2 18:37:48 UTC 2015
On Thu, Jul 02, 2015 at 12:19:24PM -0600, Ian Lepore wrote:
> 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.
>
Well, it really depends how much you want to nitpick here.
A joke problem is that By the time you open group_dir, it could have ben
renamed/mounted upon/whatever and you are opening a different directory.
A non-joke problem is that the caller cannot distinguish between failed
rename and failed open/sync, making it impossible to perform any kind of
recovery.
I would say the chane should be left as it is. It reportedly fixes the
problem encounterd in the wild and is not that bad
One has to note this entire api and pw(8) are screaming for a rewrite
for a long time now.
> -- 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
> > >
>
>
> _______________________________________________
> svn-src-all at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"
--
Mateusz Guzik <mjguzik gmail.com>
More information about the svn-src-all
mailing list