Re: Locking the passwd subsystem (was Re: bug in pw, -STABLE [patch])
And here's a run at Stage 2. This adapts a subset of programs to use the
pid_*() locking supplied. The attached patch updates: , libutil
(libutil.h and pw_util.c), chpass (chpass.c), pw (pw.c), pwd_mkdb
(Makefile, pwd_mkdb.8, pwd_mkdb.c), vipw (vipw.c). This does NOT include
passwd(1), since that does its dirty work in PAM, which I definately
don't feel like delving into.
Note that this patch MAY be a little off when applied over the previous
patch; I'm working on them in seperate trees, to try and keep the stages
discrete. But a little manual fiddling should get them reconciled in
short order.
--
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: include/pwd.h
===
RCS file: /usr/cvs/src/include/pwd.h,v
retrieving revision 1.12
diff -u -r1.12 pwd.h
--- include/pwd.h 9 Jun 2002 19:39:18 - 1.12
+++ include/pwd.h 23 Jun 2002 18:16:21 -
@@ -66,6 +66,9 @@
#define_PATH_MASTERPASSWD "/etc/master.passwd"
#define_MASTERPASSWD "master.passwd"
+#define_PATH_PWDLOCK "/var/run/pwd.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"
Index: lib/libutil/libutil.h
===
RCS file: /usr/cvs/src/lib/libutil/libutil.h,v
retrieving revision 1.37
diff -u -r1.37 libutil.h
--- lib/libutil/libutil.h 8 May 2002 00:50:07 - 1.37
+++ lib/libutil/libutil.h 23 Jun 2002 18:38:18 -
@@ -95,6 +95,8 @@
intpw_init(const char *_dir, const char *_master);
char *pw_make(struct passwd *_pw);
intpw_mkdb(const char *_user);
+intpw_globlock(int);
+intpw_globunlock(void);
intpw_lock(void);
struct passwd *pw_scan(const char *_line, int _flags);
const char *pw_tempname(void);
@@ -124,6 +126,9 @@
#defineFPARSELN_UNESCCOMM 0x04
#defineFPARSELN_UNESCREST 0x08
#defineFPARSELN_UNESCALL 0x0f
+
+/* pw_globlock() */
+#definePW_NOBLOCK PID_NOBLOCK
/* pw_scan() */
#define PWSCAN_MASTER 0x01
Index: lib/libutil/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
--- lib/libutil/pw_util.c 8 May 2002 14:52:32 - 1.25
+++ lib/libutil/pw_util.c 23 Jun 2002 18:46:19 -
@@ -79,6 +79,7 @@
static char passwd_dir[PATH_MAX];
static char tempname[PATH_MAX];
static int initialized;
+static int globlocked = 0;
void
pw_cont(int sig)
@@ -163,6 +164,28 @@
}
/*
+ * Lock the password subsystem globally.
+ */
+int
+pw_globlock(int flags)
+{
+ int retval;
+
+ if( (retval=pid_begin(_PATH_PWDLOCK, _MODE_PWDLOCK, flags)) == 0 );
+ globlocked=1;
+ return(retval);
+}
+
+/*
+ * Unlock the global lock
+ */
+int pw_globunlock(void)
+{
+
+ return(pid_end(_PATH_PWDLOCK));
+}
+
+/*
* Lock the master password file.
*/
int
@@ -258,10 +281,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:
@@ -353,6 +376,11 @@
}
if (lockfd != -1)
close(lockfd);
+ if (globlocked != 0)
+ {
+ globlocked=0;
+ pid_end(_PATH_PWDLOCK);
+ }
errno = serrno;
}
Index: usr.bin/chpass/chpass.c
===
RCS file: /usr/cvs/src/usr.bin/chpass/chpass.c,v
retrieving revision 1.23
diff -u -r1.23 chpass.c
--- usr.bin/chpass/chpass.c 8 May 2002 00:54:28 - 1.23
+++ usr.bin/chpass/chpass.c 23 Jun 2002 18:42:25 -
@@ -258,6 +258,8 @@
case _PWF_FILES:
if (pw_init(NULL, NULL))
err(1, "pw_init()");
+ if (pw_globlock(PW_NOBLOCK) < 0)
+ err(1, "pw_globlock()");
if ((pfd = pw_lock()) == -1) {
pw_fini();
err(1, "pw_lock()");
Index: usr.sbin/pw/pw.c
===
RCS file: /usr/
Re: Locking the passwd subsystem (was Re: bug in pw, -STABLE [patch])
On Sun, Jun 23, 2002 at 11:17:35AM -0400 I heard the voice of
Geoffrey C. Speicher, and lo! it spake thus:
>
> 5-minute window. Still unliklely, but not quite as unlikely as the
> non-daemon scenario.
And it all goes out when window when kern.randompid=1, so it's kinda
moot.
> Looks good from here. I've attached some minor patches (also relative to
> src/) to pid_util.{3,c}. The first corrects some minor typos in the man
> page and the second is to make pid_begin() work as advertised when read()
> fails (set errno and return -1 rather than return errno).
Yes, I have a brain. No, really I do. I just don't have enough coffee
:)
> (I haven't tested any of this yet either.)
I just slapped together a few quick test programs. Here's an updated
patch, incorporating your fixes, as well as a bugfix and an almost-bugfix
my testing showed up (hint: just because you ftruncate() to 0-length,
doesn't mean your offset becomes 0 too, dangit), and eliminated an extra
variable.
--
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: lib/libutil/Makefile
===
RCS file: /usr/cvs/src/lib/libutil/Makefile,v
retrieving revision 1.46
diff -u -r1.46 Makefile
--- lib/libutil/Makefile8 May 2002 00:50:07 - 1.46
+++ lib/libutil/Makefile23 Jun 2002 13:31:01 -
@@ -8,7 +8,7 @@
CFLAGS+=-DINET6
SRCS= _secure_path.c auth.c fparseln.c login.c login_auth.c \
login_cap.c login_class.c login_crypt.c login_ok.c login_times.c \
- login_tty.c logout.c logwtmp.c property.c pty.c \
+ login_tty.c logout.c logwtmp.c pid_util.c property.c pty.c \
pw_util.c realhostname.c stub.c \
trimdomain.c uucplock.c
INCS= libutil.h login_cap.h
@@ -16,7 +16,7 @@
MAN+= login.3 login_auth.3 login_tty.3 logout.3 logwtmp.3 pty.3 \
login_cap.3 login_class.3 login_times.3 login_ok.3 \
_secure_path.3 uucplock.3 property.3 auth.3 realhostname.3 \
- realhostname_sa.3 trimdomain.3 fparseln.3
+ realhostname_sa.3 trimdomain.3 fparseln.3 pid_util.3
MAN+= login.conf.5 auth.conf.5
MLINKS+= property.3 properties_read.3 property.3 properties_free.3
MLINKS+= property.3 property_find.3
@@ -39,5 +39,6 @@
MLINKS+=login_auth.3 auth_checknologin.3 login_auth.3 auth_cat.3
MLINKS+=uucplock.3 uu_lock.3 uucplock.3 uu_lock_txfr.3 \
uucplock.3 uu_unlock.3 uucplock.3 uu_lockerr.3
+MLINKS+=pid_util.3 pid_begin.3 pid_end.3
.include
Index: lib/libutil/libutil.h
===
RCS file: /usr/cvs/src/lib/libutil/libutil.h,v
retrieving revision 1.37
diff -u -r1.37 libutil.h
--- lib/libutil/libutil.h 8 May 2002 00:50:07 - 1.37
+++ lib/libutil/libutil.h 23 Jun 2002 12:35:30 -
@@ -82,6 +82,8 @@
struct sockaddr;
intrealhostname_sa(char *host, size_t hsize, struct sockaddr *addr,
int addrlen);
+int pid_begin(const char *pidfile, mode_t mode, int flags);
+int pid_end(const char *pidfile);
#ifdef _STDIO_H_ /* avoid adding new includes */
char *fparseln(FILE *, size_t *, size_t *, const char[3], int);
#endif
@@ -128,5 +130,8 @@
/* pw_scan() */
#define PWSCAN_MASTER 0x01
#define PWSCAN_WARN0x02
+
+/* pid_begin() */
+#define PID_NOBLOCK0x01
#endif /* !_LIBUTIL_H_ */
Index: lib/libutil/pid_util.c
===
--- /dev/null Sun Jun 23 11:44:00 2002
+++ lib/libutil/pid_util.c Sun Jun 23 11:50:59 2002
@@ -0,0 +1,178 @@
+/*-
+ * Copyright (c) 2002 Matthew D. Fuller <[EMAIL PROTECTED]>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHORS AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHORS OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABI
Re: Locking the passwd subsystem (was Re: bug in pw, -STABLE [patch])
On Sun, 23 Jun 2002, Matthew D. Fuller wrote:
> Well, a daemon would have a BETTER chance, though nowhere near a
> guarantee. The theory being that the daemon would check every so often
> (5 minutes sounds good to me), whereas with the library, you potentially
> could not check for days, depending on how often you run pw(8). Even on
> a heavily used system, 5 minutes wouldn't likely be enough to reuse a
> PID, unless you had random PID assignment or some such.
Right, for programs like pw(8), the daemon should definitely suffice for
all practical purposes. For daemons, it's another story. You don't
necessarily have to wrap around the pid list inside of those 5 minutes.
If the original program runs for a long time (which is likely for
daemons), then the pid list could wrap around while it is still running.
Then it is possible that it dies and its pid gets reuser within that
5-minute window. Still unliklely, but not quite as unlikely as the
non-daemon scenario.
> > > Now, this is a problem. There's a race condition here. It's a very
> > > small window, to be sure, but I'm not quite sure how to close it. After
> >
> > I think you must've figured it out judging by your post that just came
> > through as I was writing that previous paragraph. :)
>
> I think so. See comments in the code.
I think so too. I guess we need those flock-style locks after all.
> Code is reasonably well commented. Manpage included. Shaken, not
> stirred. Slippery when wet. This has gone through a compile test, but
> hasn't been live-tested.
Looks good from here. I've attached some minor patches (also relative to
src/) to pid_util.{3,c}. The first corrects some minor typos in the man
page and the second is to make pid_begin() work as advertised when read()
fails (set errno and return -1 rather than return errno).
(I haven't tested any of this yet either.)
You da man.
Geoff
--- lib/libutil/pid_util.3.orig Sun Jun 23 10:31:39 2002
+++ lib/libutil/pid_util.3 Sun Jun 23 10:35:22 2002
@@ -53,7 +53,7 @@
still exists.
If the process is still alive,
.Fn pid_begin
-will block and continue to try aquiring the lock indefinately, unless
+will block and continue to try aquiring the lock indefinitely, unless
.Dv PID_NOBLOCK
is set in the
.Fa flags ,
@@ -63,7 +63,7 @@
will put its own process ID in the file and continue on its merry way.
.Pp
.Fn pid_end
-removed the file referenced by
+removes the file referenced by
.Fa pidfile .
Note that there is currently no protection afforded here that the process
calling
@@ -86,9 +86,9 @@
The operation was cancelled due to internal error.
.It Bq Er EWOULDBLOCK
The file is already locked by a still-live process and
-.Fa mode
+.Fa flags
includes
-.Dv PID_NOLOCK .
+.Dv PID_NOBLOCK .
.El
.Pp
The
--- lib/libutil/pid_util.c.orig Sun Jun 23 10:36:34 2002
+++ lib/libutil/pid_util.c Sun Jun 23 10:54:58 2002
@@ -92,7 +92,8 @@
{
holding==errno;
close(lockfd);
- return(holding);
+ errno=holding;
+ return(-1);
}
masterpid = (pid_t) atoi(readpid);
Locking the passwd subsystem (was Re: bug in pw, -STABLE [patch])
[ Moved to -hackers since this isn't really a STABLE-specific issue, but rather a general thing. Bcc's to -stable for continuity's sake. ] This discussion has gone on, on and off, for months now, so I'm going to recap a bit. For the people in it, we've mostly paged out the details by now, and for the people not in it, this will be necessary catchup. pw(8) can sometimes corrupt the passwd file when it's running. This is due to the fact that, while it munges things, it locks /etc/master.passwd. This fails to really work right between invocations, because as part of its dirty work, pw(8) moves master.passwd away and creates a new one from scratch. So, an external lockfile is needed. There's no real common locking among the other consumers (chpass(1), passwd(1), vipw(8), pwd_mkdb(8), etc) either, so we might as well make a big common locking mechanism out of it. And while we're doing that, it would be nifty to centralize some functions for doing locking like that, too, instead of coding it from scratch in every application that needs it. The patch on this mail accomplishes this part of the objective; once we can all settle on that, we can easily handle locking all the consumers neatly using these functions. And now, for the carrying on. On Sun, Jun 23, 2002 at 09:07:15AM -0400 I heard the voice of Geoffrey C. Speicher, and lo! it spake thus: > > Actually, now that I think about it, even a daemon wouldn't guarantee > better results in every such scenario. At first it would seem that the > daemon would get a chance to clean up the pid file long before another > process wanted it (during step 4). However, if the timing is just right > then p1 can die the moment that its pid would be reused, and chances are > that the daemon wouldn't catch it in time. Well, a daemon would have a BETTER chance, though nowhere near a guarantee. The theory being that the daemon would check every so often (5 minutes sounds good to me), whereas with the library, you potentially could not check for days, depending on how often you run pw(8). Even on a heavily used system, 5 minutes wouldn't likely be enough to reuse a PID, unless you had random PID assignment or some such. > > Now, this is a problem. There's a race condition here. It's a very > > small window, to be sure, but I'm not quite sure how to close it. After > > I think you must've figured it out judging by your post that just came > through as I was writing that previous paragraph. :) I think so. See comments in the code. (Re: Oliver's statements about the general evil of PID files) > The beauty of encapsulating the pid file operations into a library is > that > the implementation can be changed to create a socket instead of, or (more > likely) in addition to, a pid file. In fact, this may be just the thing > we need to close that "pid gets reused" hole. > > Since Matt already has the functions mostly implemented, I'll defer to > him. Well, there's nothing necessarily stopping us from adding more stuff later, of course. Personally, I've set my sights a bit lower; for the moment, I'm more concerned with "Let's not corrupt the passwd database" than with "This must never block unless totally absolutely completely necessary and should always know where its cheese is", which is what the socket-theory is pointing at. One giant code upheaval at a time :) One possibility would be to make procfs(5) show the process start time (rather than the current time) for the ctime of the /proc/pid directory. Then, one could look at the PID file, and if its mtime (not ctime; see in below code where we could use a pre-existing file) is older than the ctime of the /proc/pid directory, we'd know the PID had changed. Getting the actual process start time would require libkvm (and thus setgid kmem), or spawning ps(1) and parsing output, otherwise. ANYWAY: Attached is a patch (relative to src/) for my first run through creating the functions to handle PID-file creation, deletion, and checking-on-creation. It consists of two functions: - pid_begin(), which creates a PID file if one doesn't exist, and checks to see if the listed process is still around (taking over the PID file if it's not) if it does. - pid_end(), which deletes the PID file. Code is reasonably well commented. Manpage included. Shaken, not stirred. Slippery when wet. This has gone through a compile test, but hasn't been live-tested. This will provide the basis then to move forward and lock all the programs that access the passwd database (for writing, of course; no reason to lock readers) in a common way. -- 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: lib/libutil/Makefile === RCS file: /usr/cvs/src/lib/libut
