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) | [EMAIL PROTECTED]
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"
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;
}
