svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb
Ermal Luçi
eri at freebsd.org
Thu Jul 2 18:00:32 UTC 2015
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.
> + 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