From owner-freebsd-bugs Mon Jun 8 13:10:43 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id NAA22668 for freebsd-bugs-outgoing; Mon, 8 Jun 1998 13:10:43 -0700 (PDT) (envelope-from owner-freebsd-bugs@FreeBSD.ORG) Received: from bangkok.office.cdsnet.net (bangkok.office.cdsnet.net [204.118.245.23]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id NAA22269; Mon, 8 Jun 1998 13:10:21 -0700 (PDT) (envelope-from cts@bangkok.office.cdsnet.net) Received: (from cts@localhost) by bangkok.office.cdsnet.net (8.8.8/8.8.5) id NAA21521; Mon, 8 Jun 1998 13:10:13 -0700 (PDT) Date: Mon, 8 Jun 1998 13:10:13 -0700 (PDT) Message-Id: <199806082010.NAA21521@bangkok.office.cdsnet.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit From: Craig Spannring To: nate@mt.sri.com, phk@FreeBSD.ORG CC: freebsd-bugs@FreeBSD.ORG Subject: Re: bin/6787: race condition in pw command In-Reply-To: <199805300220.UAA28867@mt.sri.com> References: <199805291844.LAA07680@hub.freebsd.org> <199805300220.UAA28867@mt.sri.com> X-Mailer: VM 6.31 under Emacs 20.2.1 Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org These are the patches to pw for -stable. I will send the patches for -current in the next mail message. I tried to follow the same style as charnier(?). One spot where I disagree with the style is near the end of the main function in pw.c. He reuses the 'ch' variable to hold the return status of the funcs[which] (cnf, mode, &arglist) call. It overloads the meaning of 'ch' and is not very descriptive, but I left it as is. The fileupdate function is still somewhat broken. Instead of returning a failure code if it can't modify the original file it renames the .new file and continues as though nothing is wrong. This will cause the lock on the original file to be lost and could lead to a similar race condition. I left that portion of the code alone since I feel that the maintainer of the code would have a better concept of how he wants to handle errors in that function than I do. BTW- I used 'diff -c -r pw-stable.original pw-stable' to generate this patch. >---cut here---< diff -c -r pw-stable.original/edgroup.c pw-stable/edgroup.c *** pw-stable.original/edgroup.c Mon Jun 8 10:44:35 1998 --- pw-stable/edgroup.c Mon Jun 8 11:33:27 1998 *************** *** 64,70 **** int rc = 0; int infd; ! if ((infd = open(groupfile, O_RDWR | O_CREAT | O_EXLOCK, 0644)) != -1) { FILE *infp; if ((infp = fdopen(infd, "r+")) == NULL) --- 64,70 ---- int rc = 0; int infd; ! if ((infd = open(groupfile, O_RDWR | O_CREAT, 0644)) != -1) { FILE *infp; if ((infp = fdopen(infd, "r+")) == NULL) diff -c -r pw-stable.original/fileupd.c pw-stable/fileupd.c *** pw-stable.original/fileupd.c Mon Jun 8 10:44:36 1998 --- pw-stable/fileupd.c Mon Jun 8 11:40:40 1998 *************** *** 49,55 **** if (pfxlen <= 1) errno = EINVAL; else { ! int infd = open(filename, O_RDWR | O_CREAT | O_EXLOCK, fmode); if (infd != -1) { FILE *infp = fdopen(infd, "r+"); --- 49,55 ---- if (pfxlen <= 1) errno = EINVAL; else { ! int infd = open(filename, O_RDWR | O_CREAT, fmode); if (infd != -1) { FILE *infp = fdopen(infd, "r+"); *************** *** 134,142 **** fputs(line, infp); /* * This is a gross hack, but we may have * corrupted the original file ! * Unfortunately, it will lose the inode. */ if (fflush(infp) == EOF || ferror(infp)) rc = rename(file, filename) == 0; --- 134,153 ---- 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. ! * ! * The implications of this is that this invocation of pw ! * won't have the file locked and concurrent copies ! * of pw, vipw etc could clobber what this one is doing. ! * ! * It should probably just return an error instead ! * of going on like nothing is wrong. */ if (fflush(infp) == EOF || ferror(infp)) rc = rename(file, filename) == 0; diff -c -r pw-stable.original/pw.c pw-stable/pw.c *** pw-stable.original/pw.c Mon Jun 8 10:44:36 1998 --- pw-stable/pw.c Mon Jun 8 12:31:09 1998 *************** *** 31,36 **** --- 31,37 ---- #include "pw.h" #include + #include const char *Modes[] = {"add", "del", "mod", "show", "next", NULL}; const char *Which[] = {"user", "group", NULL}; *************** *** 47,52 **** --- 48,54 ---- static int getindex(const char *words[], const char *word); static void cmdhelp(int mode, int which); + static int filelock(const char *filename); int *************** *** 145,151 **** * Now, let's do the common initialisation */ cnf = read_userconfig(getarg(&arglist, 'C') ? getarg(&arglist, 'C')->val : NULL); ! return funcs[which] (cnf, mode, &arglist); } static int --- 147,172 ---- * Now, let's do the common initialisation */ cnf = read_userconfig(getarg(&arglist, 'C') ? getarg(&arglist, 'C')->val : NULL); ! ! /* ! * Be pessimistic and lock the master passowrd and group ! * files right away. Keep it locked for the duration. ! */ ! if (-1 == filelock(_PATH_GROUP) || -1 == filelock(_PATH_MASTERPASSWD)) ! { ! ch = EX_IOERR; ! } ! else ! { ! ch = funcs[which] (cnf, mode, &arglist); ! } ! return ch; ! } ! ! static int ! filelock(const char *filename) ! { ! return open(filename, O_RDONLY | O_EXLOCK, 0); } static int To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message