Date: Thu, 2 Jul 2015 20:37:41 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Ian Lepore <ian@freebsd.org> Cc: Ermal =?utf-8?B?THXDp2k=?= <eri@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Renato Botelho <garga@freebsd.org> Subject: Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb Message-ID: <20150702183741.GA7666@dft-labs.eu> In-Reply-To: <1435861164.1648.129.camel@freebsd.org> References: <201507021731.t62HV074085188@repo.freebsd.org> <CAPBZQG288Lw1nQ9BC-T6FU9c5VE4-ANGhkN0xaURdym0x_D-WQ@mail.gmail.com> <1435861164.1648.129.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/svn-src-all > To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150702183741.GA7666>