Date: Sun, 23 Jun 2002 08:54:41 -0500 From: "Matthew D. Fuller" <fullermd@over-yonder.net> To: "Geoffrey C. Speicher" <geoff@sea-incorporated.com> Cc: freebsd-hackers@freebsd.org, Matt Simerson <freebsd@blockads.com>, Paul Herman <pherman@frenchfries.net>, Oliver Fromme <olli@secnetix.de> Subject: Locking the passwd subsystem (was Re: bug in pw, -STABLE [patch]) Message-ID: <20020623135441.GC81018@over-yonder.net> In-Reply-To: <20020623084607.H29729-100000@sea-incorporated.com> References: <20020623024804.GB95458@over-yonder.net> <20020623084607.H29729-100000@sea-incorporated.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--FL5UXtIhxfXey3p5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline [ 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) | 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" --FL5UXtIhxfXey3p5 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=diffs Index: lib/libutil/Makefile =================================================================== RCS file: /usr/cvs/src/lib/libutil/Makefile,v retrieving revision 1.46 diff -u -r1.46 Makefile --- lib/libutil/Makefile 8 May 2002 00:50:07 -0000 1.46 +++ lib/libutil/Makefile 23 Jun 2002 13:31:01 -0000 @@ -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 <bsd.lib.mk> 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 -0000 1.37 +++ lib/libutil/libutil.h 23 Jun 2002 12:35:30 -0000 @@ -82,6 +82,8 @@ struct sockaddr; int realhostname_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_WARN 0x02 + +/* pid_begin() */ +#define PID_NOBLOCK 0x01 #endif /* !_LIBUTIL_H_ */ Index: lib/libutil/pid_util.c =================================================================== --- /dev/null Sun Jun 23 08:44:01 2002 +++ lib/libutil/pid_util.c Sun Jun 23 07:34:28 2002 @@ -0,0 +1,176 @@ +/*- + * Copyright (c) 2002 Matthew D. Fuller <fullermd@over-yonder.net> + * 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 LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#ifndef lint +static const char rcsid[] = + "$FreeBSD$"; +#endif /* not lint */ + +/* + * These functions are for maintenance of locking PID files + */ + +#include <sys/param.h> +#include <sys/proc.h> +#include <sys/uio.h> + +#include <errno.h> +#include <fcntl.h> +#include <signal.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +#include <libutil.h> + + +#define PID_LEN 6 /* Currently only need 5 (0-99,999) */ + +/* + * pid_begin: Open a PID file and write PID into it. If one already + * exists, try and track down the process which opened it; if it can't be + * found, proceed as if the file weren't even there. + */ +int +pid_begin(const char *pidfile, mode_t mode, int flags) +{ + int procdead=0; + int lockfd; + int holding=0; + pid_t masterpid; + char *pidstr; + char readpid[PID_LEN+1]; + +start: + if( (lockfd=open(pidfile, O_RDWR | O_CREAT | O_EXCL | O_EXLOCK, + mode)) < 0 ) + { + if(errno!=EEXIST) + return(-1); /* Preserve errno */ + + /* Open and find PID */ + if( (lockfd=open(pidfile, O_RDWR | O_EXLOCK, mode)) < 0 ) + { + if(errno==ENOENT || errno==EWOULDBLOCK) + { + /* + * This could use some thought. We're going to + * tight-loop if someone else has the file locked + * (EWOULDBLOCK). In theory, any locks should be + * quite short-lived, so I THINK this will be fine. + */ + goto start; + } + else + return(-1);/* Preserve errno */ + } + + if( read(lockfd, readpid, PID_LEN) < 0 ) + { + holding==errno; + close(lockfd); + return(holding); + } + + masterpid = (pid_t) atoi(readpid); + if(masterpid<1) /* This shouldn't happen */ + { + /* This is really a NFS error, but we'll abuse it */ + errno=ECANCELED; + return(-1); + } + + /* + * There's a number of ways we could approach this. We could + * stat /proc/$PID, but that would fail if /proc wasn't + * mounted. We could use libkvm, but that would require + * linking libutil with it, as well as being setgid kmem on + * the app (which we can't really count on). We could + * fork/exec ps(1), but that's a bit ugly. + * I'm going to do it in a rather ugly way. SIGWINCH is + * fairly harmless in the cases where it's used, and is + * ignored by default. These functions are aimed at daemons, + * which will generally have no use for it, and thus not + * change it from the default discard. + * Note that zombie'd processes count as 'nonexistent' to + * kill(2). + */ + if( kill(masterpid, SIGWINCH) < 0 ) + if(errno==ESRCH) + procdead=1; + else + procdead=0; + else + procdead=0; /* kill(2) succeeded */ + + if(procdead==0) + { + /* Old locker is still alive */ + close(lockfd); + if( (flags & PID_NOBLOCK) ) + { + errno=EWOULDBLOCK; + return(-1); + } + else + { + sleep(1); /* Arbitrary */ + goto start; + } + } + /* Old lockholder is dead: fallthru */ + } + + /* + * If we get here, we either ARE the O_EXCL opener, or the process + * listed in the file is dead and we're taking it over. + */ + ftruncate(lockfd, 0); + write( lockfd, pidstr, asprintf(&pidstr, "%d", getpid()) ); + free(pidstr); + + close(lockfd); + return(0); +} + + +/* + * pid_end: Clean up a lock we hold. + */ +int +pid_end(const char *pidfile) +{ + /* + * We SHOULDN'T need to aquire a lock on the file, since the only + * time anybody else should be writing into it would be if we're + * dead, and thus we wouldn't be in this function. If they're just + * reading it, unlink()'ing it out from under them won't do any + * damage. If we're still alive, pid_begin() should close() the file + * and attempt to re-open() for the next attempt, so this should work + * nice and cleanly. + */ + return(unlink(pidfile)); +} Index: lib/libutil/pid_util.3 =================================================================== --- /dev/null Sun Jun 23 08:44:01 2002 +++ lib/libutil/pid_util.3 Sun Jun 23 08:29:49 2002 @@ -0,0 +1,106 @@ +.\" Copyright (c) 2002 Matthew D. Fuller <fullermd@over-yonder.net> +.\" 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 AUTHOR 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 AUTHOR 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 LIABILITY, WHETHER IN CONTRACT, STRICT +.\" LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +.\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +.\" SUCH DAMAGE. +.\" +.\" $FreeBSD$ +.\" +.Dd June 23, 2002 +.Os +.Dt PID_UTIL 3 +.Sh NAME +.Nm pid_begin , +.Nm pid_end +.Nd "handle locking and unlocking of PID files" +.Sh LIBRARY +.Lb libutil +.Sh SYNOPSIS +.In sys/types.h +.In libutil.h +.Ft int +.Fn pid_begin "const char *pidfile" "mode_t mode" "int flags" +.Ft int +.Fn pid_end "const char *pidfile" +.Sh DESCRIPTION +The function +.Fn pid_begin +attempts to open the file referenced by +.Fa pidfile +and write the process ID to it. +If the file already exists, +.Fn pid_begin +will determine if the process listed in +.Fa pidfile +still exists. +If the process is still alive, +.Fn pid_begin +will block and continue to try aquiring the lock indefinately, unless +.Dv PID_NOBLOCK +is set in the +.Fa flags , +in which case it will return an error. +If the old process is dead, or the file doesn't exist, +.Fn pid_begin +will put its own process ID in the file and continue on its merry way. +.Pp +.Fn pid_end +removed the file referenced by +.Fa pidfile . +Note that there is currently no protection afforded here that the process +calling +.Fn pid_end +is actually the process that opened the PID file in the first place. +.Sh RETURN VALUES +If successful, +.Fn pid_begin +and +.Fn pid_end +will return 0. +They will return -1 on failure, and set +.Va errno +to indicate the error. +.Sh ERRORS +.Fn pid_begin +will leave behind a PID file unless: +.Bl -tag -width Er +.It Bq Er ECANCELED +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 +includes +.Dv PID_NOLOCK . +.El +.Pp +The +.Fn pid_begin +function may also fail and set errno for any of the errors specified for +the routines +.Xr open 2 +or +.Xr read 2 . +.Pp +The +.Fn pid_end +function may fail and set errno for any of the errors specified for the +routine +.Xr unlink 2 . --FL5UXtIhxfXey3p5-- 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?20020623135441.GC81018>