From owner-svn-src-head@freebsd.org Thu Jul 2 18:00:32 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9D852993448; Thu, 2 Jul 2015 18:00:32 +0000 (UTC) (envelope-from ermal.luci@gmail.com) Received: from mail-yk0-x231.google.com (mail-yk0-x231.google.com [IPv6:2607:f8b0:4002:c07::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 5EAD019A5; Thu, 2 Jul 2015 18:00:32 +0000 (UTC) (envelope-from ermal.luci@gmail.com) Received: by ykfy125 with SMTP id y125so75479755ykf.1; Thu, 02 Jul 2015 11:00:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=dw6mr1hAXK9peN5PI/KJ4vEZXdz3sa2385Z1z6q3MME=; b=VLlkCjP7cnOxO3dRjs9cY3jXz7zc0/28IuOOBYkEi/W6fFqTd72dR8T34aRrJHLCQq bMk3qXPfhbCZC4GnBEx/qWPAyeDdxL84AMxGN6TPOZuJXmbXxnnvE/LJpDxqNth3EFWO ZwXnjXitseQcncn596pk/3jydSZ+aR8n7Fmj21gZZSBc9at9EL53hG4FtGTlNeTbp8rZ hVt8V+w0p1G9X5YFsg9uwYVfKxp6pKl6oMaNJPh3oxEL41flvS52JXkgBEjZvMKuqquS /Ay5yKmKDQs1cxHYatBttnkhUOoF1u/3jxVVPTMhX8SdEODup9RDCWexnwcB7i3Oq13V hoDQ== MIME-Version: 1.0 X-Received: by 10.129.82.130 with SMTP id g124mr40856286ywb.9.1435860031435; Thu, 02 Jul 2015 11:00:31 -0700 (PDT) Sender: ermal.luci@gmail.com Received: by 10.129.83.139 with HTTP; Thu, 2 Jul 2015 11:00:31 -0700 (PDT) In-Reply-To: <201507021731.t62HV074085188@repo.freebsd.org> References: <201507021731.t62HV074085188@repo.freebsd.org> Date: Thu, 2 Jul 2015 20:00:31 +0200 X-Google-Sender-Auth: STK1NdlFkIol8c6wdOgC8p00cC8 Message-ID: Subject: Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb From: =?UTF-8?Q?Ermal_Lu=C3=A7i?= To: Renato Botelho Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Jul 2015 18:00:32 -0000 On Thu, Jul 2, 2015 at 7:31 PM, Renato Botelho 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 > #include > #include > +#include > #include > #include > #include > @@ -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 >