Date: Mon, 17 Jun 2002 23:29:39 -0500 From: "Matthew D. Fuller" <fullermd@over-yonder.net> To: "Geoffrey C. Speicher" <geoff@sea-incorporated.com> Cc: freebsd-stable@freebsd.org, Matt Simerson <freebsd@blockads.com> Subject: Re: bug in pw, -STABLE [patch] Message-ID: <20020618042939.GF72664@over-yonder.net> In-Reply-To: <Pine.BSF.4.10.10205171825320.67053-300000@sea-incorporated.com> References: <Pine.BSF.4.10.10205171825320.67053-300000@sea-incorporated.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Sorry for the slooooooow response, I've had problems you wouldn't believe with my email... On Fri, May 17, 2002 at 06:40:57PM -0400 I heard the voice of Geoffrey C. Speicher, and lo! it spake thus: > > 1. The lock isn't very fine-grained. We grab one giant lock before > doing any operations at all, and let it go right before we exit. > The alternative is to lock for /etc/master.passwd and /etc/group > individually as necessary, but I saw no point in going that far since > pw seems to spend most of its time updating those files anyway. This was what I had in mind in the first place; just a single fairly coarse lock on anything changing [master.]passwd. I hadn't actually thought of group, mainly because I don't subscribe to the group-per-user philosophy, so group changes are practically nonexistent and almost always manual (but then, I add users with vipw too ;), but a single lock around them makes sense to me. A "Don't futz with user auth because I'm fiddling with it" lock. > 2. The lockfile is /var/run/pw.lock and it always contains the pid of > the pw process that most recently grabbed the lock. Good, good... > 3. The lockfile is never deleted, because doing so seemed to cause a > race condition between the time the file was closed and unlinked, > leading to password file corruption again. If anyone has a solution > to this, please speak up. Sure; unlink it before you close it. However, we can avoid that by using O_CREAT | O_EXCL (rather than using O_EXLOCK, which uses flock(2)-style stuff). That way, if the file exists, it's locked, so drop out. However, I'd make a few changes and additions. First off, I'd say put the lock info in <pwd.h>, not in a local include for pw(8). See first attachment. The naming was changed to match the other stuff in that file, and changed to do a create/delete sequence as well, so see second attachment for updated diffs to pw(8). Now, that brings us up-to-date. I'd also propose the next step (which should be the same step) would be to update a few other things; for instance, passwd(1), chpass(1), pwd_mkdb(8), vipw(8), etc. From what I can see of the source, all of them use stuff from libutil (why doesn't pw(8)? Oh well, one more thing to patch down the road). So, accordingly, see third attachment for patches to libutil (note that this changes the way pw_mkdb() calls pwd_mkdb(8), so it depends on the next patch). pwd_mkdb(8) needs to try grabbing the auth subsystem lock, EXCEPT when it's already held (duh); this COULD be done by having it try and see if the lock is held by it's parent by checking the PID, but I'm too lazy, so I'll add a command-line option; see fourth attachment for those diffs. vipw(8) needs the minor addition of lock/unlock, so that's attachment 5. passwd(1) uses PAM, which I'm fairly sure I don't want to get into right now, so I'll ignore it for the time being. *pant* OK. All these compile, though I haven't run them all through a torture test. For my next trick, I'm going to redo adduser(8) and rmuser(8) so they use pw(8) to do their dirty work, but I'm going to take a slight break before I consider THAT mess. So, to recap attachments: 1- Updated /usr/include/pwd.h with locking file/mode 2- Updated pw(8) with O_EXCL change and changed names for #define's 3- Updated libutil (requires patch #4 to work right) with lock/unlock functions and changed calling of pwd_mkdb(8) 4- Updated pwd_mkdb(8) with locking, EXCEPT when called with new arg '-n', which is used by libutil in pw_mkdb() (see patch #3), which is the function used by, e.g., vipw(8) 5- Updated vipw(8) How does that all strike you? I'm probably going to fiddle with this a bit later to try and see how badly it breaks, but if it works out, then IMHO it's a good first step to consolidating all the passwd-manipulation cruft. -- Matthew Fuller (MF4839) | fullermd@over-yonder.net Systems/Network Administrator | http://www.over-yonder.net/~fullermd/ "The only reason I'm burning my candle at both ends, is because I haven't figured out how to light the middle yet" --6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="pwd.diffs" Index: pwd.h =================================================================== RCS file: /usr/cvs/src/include/pwd.h,v retrieving revision 1.12 diff -u -r1.12 pwd.h --- pwd.h 9 Jun 2002 19:39:18 -0000 1.12 +++ pwd.h 18 Jun 2002 03:26:38 -0000 @@ -66,6 +66,9 @@ #define _PATH_MASTERPASSWD "/etc/master.passwd" #define _MASTERPASSWD "master.passwd" +#define _PATH_PWDLOCK "/var/run/pw.lock" +#define _MODE_PWDLOCK (S_IRUSR | S_IWUSR) + #define _PATH_MP_DB "/etc/pwd.db" #define _MP_DB "pwd.db" #define _PATH_SMP_DB "/etc/spwd.db" --6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="pw.patch" Index: pw.c =================================================================== RCS file: /usr/cvs/src/usr.sbin/pw/pw.c,v retrieving revision 1.26 diff -u -r1.26 pw.c --- pw.c 9 Jul 2001 09:24:01 -0000 1.26 +++ pw.c 18 Jun 2002 03:48:16 -0000 @@ -33,6 +33,7 @@ #include <fcntl.h> #include <locale.h> #include <paths.h> +#include <pwd.h> #include <sys/wait.h> #include "pw.h" @@ -98,10 +99,12 @@ main(int argc, char *argv[]) { int ch; + int lockfd; int mode = -1; int which = -1; char *config = NULL; struct userconf *cnf; + char *pidstr; static const char *opts[W_NUM][M_NUM] = { @@ -202,6 +205,17 @@ errx(EX_NOPERM, "you must be root to run this program"); /* + * Gain exclusive lock before updating any files + */ + lockfd = open(_PATH_PWDLOCK, O_WRONLY | O_CREAT | O_EXCL, _LOCK_FILE_MODE); + if (lockfd == -1) + err(EX_UNAVAILABLE, "%s", _PATH_PWDLOCK); + + write(lockfd, pidstr, asprintf(&pidstr, "%d", getpid())); + free(pidstr); + close(lockfd); + + /* * We should immediately look for the -q 'quiet' switch so that we * don't bother with extraneous errors */ @@ -259,6 +273,12 @@ pw_log(cnf, mode, which, "NIS maps updated"); } } + + /* + * Release the lock + */ + unlink(lockfd); + return ch; } --6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="libutil.patch" Index: libutil.h =================================================================== RCS file: /usr/cvs/src/lib/libutil/libutil.h,v retrieving revision 1.37 diff -u -r1.37 libutil.h --- libutil.h 8 May 2002 00:50:07 -0000 1.37 +++ libutil.h 18 Jun 2002 03:53:02 -0000 @@ -95,6 +95,8 @@ int pw_init(const char *_dir, const char *_master); char *pw_make(struct passwd *_pw); int pw_mkdb(const char *_user); +int pw_authlock(void); +int pw_authunlock(void); int pw_lock(void); struct passwd *pw_scan(const char *_line, int _flags); const char *pw_tempname(void); Index: pw_util.c =================================================================== RCS file: /usr/cvs/src/lib/libutil/pw_util.c,v retrieving revision 1.25 diff -u -r1.25 pw_util.c --- pw_util.c 8 May 2002 14:52:32 -0000 1.25 +++ pw_util.c 18 Jun 2002 04:16:00 -0000 @@ -163,6 +163,38 @@ } /* + * Lock the auth subsystem. + */ +int +pw_authlock(void) +{ + char *pidstr; + + /* + * This wraps a lock around everything, passwd and group related. + * Not everything uses this, but everything should be converted to. + */ + lockfd = open(_PATH_PWDLOCK, O_WRONLY | O_CREAT | O_EXCL, _MODE_PWDLOCK); + if (lockfd == -1) + errx(1, "the password db file is locked"); + + write(lockfd, pidstr, asprintf(&pidstr, "%d", getpid())); + free(pidstr); + close(lockfd); + + return(0); +} + +/* + * Unlock the auth subsystem. + */ +int +pw_authunlock(void) +{ + return(unlink(_PATH_PWDLOCK)); +} + +/* * Lock the master password file. */ int @@ -258,10 +290,10 @@ case 0: /* child */ if (user == NULL) - execl(_PATH_PWD_MKDB, "pwd_mkdb", "-p", + execl(_PATH_PWD_MKDB, "pwd_mkdb", "-p", "-n", "-d", passwd_dir, tempname, NULL); else - execl(_PATH_PWD_MKDB, "pwd_mkdb", "-p", + execl(_PATH_PWD_MKDB, "pwd_mkdb", "-p", "-n", "-d", passwd_dir, "-u", user, tempname, NULL); _exit(1); default: --6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="pwd_mkdb.patch" Index: Makefile =================================================================== RCS file: /usr/cvs/src/usr.sbin/pwd_mkdb/Makefile,v retrieving revision 1.8 diff -u -r1.8 Makefile --- Makefile 20 Jul 2001 06:20:15 -0000 1.8 +++ Makefile 18 Jun 2002 04:12:21 -0000 @@ -8,5 +8,7 @@ SRCS= pw_scan.c pwd_mkdb.c CFLAGS+= -I${.CURDIR}/../../lib/libc/gen # for pw_scan.h +DPADD= ${LIBUTIL} +LDADD= -lutil .include <bsd.prog.mk> Index: pwd_mkdb.8 =================================================================== RCS file: /usr/cvs/src/usr.sbin/pwd_mkdb/pwd_mkdb.8,v retrieving revision 1.19 diff -u -r1.19 pwd_mkdb.8 --- pwd_mkdb.8 16 May 2002 02:28:35 -0000 1.19 +++ pwd_mkdb.8 18 Jun 2002 04:15:13 -0000 @@ -42,6 +42,7 @@ .Nm .Op Fl C .Op Fl N +.Op Fl n .Op Fl p .Op Fl d Ar directory .Op Fl s Ar cachesize @@ -76,6 +77,13 @@ to exit with an error if it cannot obtain a lock on the file. By default, we block waiting for a lock on the source file. The lock is held through the rebuilding of the database. +.It Fl n +Tell +.Nm +to not attempt to grab the auth subsystem lock. This is really intended +only to be used internally by programs such as +.Xr pwd_mkdb 8 ; +use with caution. .It Fl p Create a Version 7 style password file and install it into .Pa /etc/passwd . Index: pwd_mkdb.c =================================================================== RCS file: /usr/cvs/src/usr.sbin/pwd_mkdb/pwd_mkdb.c,v retrieving revision 1.38 diff -u -r1.38 pwd_mkdb.c --- pwd_mkdb.c 9 Mar 2002 03:52:14 -0000 1.38 +++ pwd_mkdb.c 18 Jun 2002 04:10:22 -0000 @@ -59,6 +59,7 @@ #include <stdlib.h> #include <string.h> #include <unistd.h> +#include <libutil.h> #include "pw_scan.h" @@ -111,6 +112,7 @@ u_int method, methoduid; int Cflag; int nblock = 0; + int nolock = 0; Cflag = 0; strcpy(prefix, _PATH_PWD); @@ -138,6 +140,9 @@ case 'N': /* do not wait for lock */ nblock = LOCK_NB; break; + case 'n': /* do not authlock */ + nolock = 1; + break; default: usage(); } @@ -178,6 +183,9 @@ if (!(fp = fopen(pname, "r"))) error(pname); + if (nolock != 1) + if(pw_authlock() < 0) + error("pw_authlock"); if (flock(fileno(fp), LOCK_EX|nblock) < 0) error("flock"); if (fstat(fileno(fp), &st) < 0) @@ -484,6 +492,10 @@ */ if (fclose(fp) == EOF) error("close fp"); + + if (nolock != 1) + if(pw_authunlock() < 0) + error("pw_authunlock"); exit(0); } --6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="vipw.diffs" Index: vipw.c =================================================================== RCS file: /usr/cvs/src/usr.sbin/vipw/vipw.c,v retrieving revision 1.13 diff -u -r1.13 vipw.c --- vipw.c 8 May 2002 00:54:28 -0000 1.13 +++ vipw.c 18 Jun 2002 04:19:51 -0000 @@ -96,6 +96,10 @@ pw_fini(); err(1, "pw_lock()"); } + if (pw_authlock() < 0) { + pw_fini(); + err(1, "pw_authlock()"); + } if ((tfd = pw_tmp(pfd)) == -1) { pw_fini(); err(1, "pw_tmp()"); @@ -128,6 +132,7 @@ if (len > 0 && (*line == 'N' || *line == 'n')) break; } + pw_authunlock(); pw_fini(); exit(0); } --6c2NcOVqGQ03X4Wi-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-stable" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020618042939.GF72664>