Date: Sun, 23 Jun 2002 14:24:57 -0500 From: "Matthew D. Fuller" <fullermd@over-yonder.net> To: Paul Herman <pherman@frenchfries.net> Cc: "Geoffrey C. Speicher" <geoff@sea-incorporated.com>, freebsd-hackers@FreeBSD.ORG Subject: Re: bug in pw, -STABLE [patch] Message-ID: <20020623192457.GJ81018@over-yonder.net> In-Reply-To: <20020623111412.V38509-100000@mammoth.eat.frenchfries.net> References: <20020623170452.GG81018@over-yonder.net> <20020623111412.V38509-100000@mammoth.eat.frenchfries.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--yudcn1FV7Hsu/q59 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Jun 23, 2002 at 11:32:54AM -0700 I heard the voice of Paul Herman, and lo! it spake thus: > > In fact, if you look at fileupdate(), you see that it already gains > an exclusive lock on the temp file, but not the original > "/etc/master.passwd" (if you will.) I think this is a bug, because > the original is getting modified (at least in name space), so that > should be locked while pw(8) is operating. I'm not sure. Hm. I think that MAY prevent some of the corruption cases. On the other hand, we're really addressing a broader spectrum of issues here. The was pw(8) manipulates the passwd database is rather different from how chpass(1) or vipw(8) do it; this unifies it all (well, starts the process). Also, looking down, it seems that pw(8) will ALWAYS try to do a line-by-line copy of the temp file back into the main file, which would be Bad Bad Bad (tm); it does this because using rename() all the time loses the flock() lock. However, if we use this global locking, we can dispense with that, and remove some code complexity, to say nothing of a giant waste of time copying line-by-line. I'm trying to kill several birds with as small a stone as I can; I've had my eye on this whole subsystem for a while, and I'd really like to re-center it all around pw(8). vipw(8) kinda has to be its own beast, but chpass(1) and adduser(8)/rmuser(8) are prime candidates to be reworked to use pw(8) for their dirty work. Let's try this: updated patch for pw(8) including my global locking, the addition of flock()'ing both old and new files (it's not quite necessary, since it's all under a global lock, but it never hurts to double-check) as in your patch, and removing the line-by-line copy and using rename() all the time. (I suggest tabstop=4 or even =2; that file is *DEEP*) -- 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" --yudcn1FV7Hsu/q59 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="pw.diffs" Index: fileupd.c =================================================================== RCS file: /usr/cvs/src/usr.sbin/pw/fileupd.c,v retrieving revision 1.9 diff -u -r1.9 fileupd.c --- fileupd.c 26 Oct 1999 04:27:13 -0000 1.9 +++ fileupd.c 23 Jun 2002 19:22:16 -0000 @@ -76,7 +76,8 @@ if (pfxlen <= 1) rc = EINVAL; else { - int infd = open(filename, O_RDWR | O_CREAT, fmode); + int infd = open(filename, + O_RDWR | O_CREAT | O_EXLOCK | O_NONBLOCK, fmode); if (infd == -1) rc = errno; @@ -92,7 +93,8 @@ strcpy(file, filename); strcat(file, ".new"); - outfd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_EXLOCK, fmode); + outfd = open(file, + O_RDWR | O_CREAT | O_TRUNC | O_EXLOCK | O_NONBLOCK, fmode); if (outfd == -1) rc = errno; else { @@ -169,27 +171,24 @@ rc = errno; /* Failed to update */ else { /* - * Copy data back into the + * Move data back into the * original file and truncate */ rewind(infp); rewind(outfp); - while (fgets(line, linesize, outfp) != NULL) - fputs(line, infp); /* - * If there was a problem with copying - * we will just rename 'file.new' - * to 'file'. - * This is a gross hack, but we may have - * corrupted the original file - * Unfortunately, it will lose the inode - * and hence the lock. + * Use rename() to move the new file over + * in place of the old file. This will + * lose the flock() locks we have, but + * this is all done inside the global + * passwd subsystem lock, so that's fine; + * it's also a lot more efficient, + * especially as the passwd file gets + * bigger and bigger, than copying a line + * at a time. */ - if (fflush(infp) == EOF || ferror(infp)) - rename(file, filename); - else - ftruncate(infd, ftell(infp)); + rename(file, filename); } } free(line); 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 23 Jun 2002 18:35:28 -0000 @@ -31,8 +31,10 @@ #include <err.h> #include <fcntl.h> +#include <libutil.h> #include <locale.h> #include <paths.h> +#include <pwd.h> #include <sys/wait.h> #include "pw.h" @@ -202,6 +204,12 @@ errx(EX_NOPERM, "you must be root to run this program"); /* + * Grab global lock before doing anything + */ + if(pid_begin(_PATH_PWDLOCK, _MODE_PWDLOCK, PID_NOBLOCK) < 0) + err(EX_UNAVAILABLE, "%s", _PATH_PWDLOCK); + + /* * We should immediately look for the -q 'quiet' switch so that we * don't bother with extraneous errors */ @@ -259,6 +267,10 @@ pw_log(cnf, mode, which, "NIS maps updated"); } } + + /* Release the lock */ + pid_end(_PATH_PWDLOCK); + return ch; } --yudcn1FV7Hsu/q59-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020623192457.GJ81018>