Date: Mon, 8 Jun 1998 13:10:13 -0700 (PDT) From: Craig Spannring <cts@internetcds.com> To: nate@mt.sri.com, phk@FreeBSD.ORG Cc: freebsd-bugs@FreeBSD.ORG Subject: Re: bin/6787: race condition in pw command Message-ID: <199806082010.NAA21521@bangkok.office.cdsnet.net> In-Reply-To: <199805300220.UAA28867@mt.sri.com> References: <199805291844.LAA07680@hub.freebsd.org> <199805300220.UAA28867@mt.sri.com>
index | next in thread | previous in thread | raw e-mail
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 <err.h>
+ #include <fcntl.h>
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
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199806082010.NAA21521>