Date: Sun, 23 Jun 2002 16:06:36 -0500 From: "Matthew D. Fuller" <fullermd@over-yonder.net> To: Paul Herman <pherman@frenchfries.net> Cc: "Geoffrey C. Speicher" <geoff@sea-incorporated.com>, freebsd-hackers@FreeBSD.ORG Subject: Re: bug in pw, -STABLE [patch] Message-ID: <20020623210636.GK81018@over-yonder.net> In-Reply-To: <20020623124243.H38673-100000@mammoth.eat.frenchfries.net> References: <20020623192457.GJ81018@over-yonder.net> <20020623124243.H38673-100000@mammoth.eat.frenchfries.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jun 23, 2002 at 01:19:02PM -0700 I heard the voice of
Paul Herman, and lo! it spake thus:
>
> I think you might have your infp confused with your outfp. It's
> not writing to the original "live" file, it's just writing the new
> temp file. That part of the code is OK.
I'm talking about down around line 177 thru (unpatched), where it's
copying back to the primary file. As it stands now (I hadn't realized in
my prior reading) it's line-by-line'ing it:
/*
* Copy 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.
*/
if (fflush(infp) == EOF || ferror(infp))
rename(file, filename);
else
ftruncate(infd, ftell(infp));
which is Very Bad (tm) in that it's not very resilient against system
crashes. The rename() solution is much safer.
> As for the giant lock, it would be like closing down the entire
> airport after someone finds a knife in the snack bar, and in my
> opinion an this is an unwise and brutal thing to do.
No, it's more like closing the snack bar while you clean it, rather than
closing the first steam tray while cleaning that, then the second steam
tray, then the third booth on the left side, then... That way, when you
open the snack bar, you know that the whole thing is clean (or, depending
on your skill and work ethic, at least equally dirty ;).
In many passwd-manipulations, the file can't be safely modified in-place,
so it has to be done out-of-place, then atomically shoved in, which
requires a rename() or the ilk; the downside is that you can't flock()
the file in question and handle that easily. One also has to consider
the impact with other programs, potentially not written by us, that
modify things; a wrapper lock on "changes to the auth information" is
really the only way to ensure consistency; otherwise, you can't REALLY be
sure that the passwd and group files are in sync, to say nothing of
preventing crashes from mangling things.
> Not to mention stale /var/run/ files that might be lying around...
See the code I put in pid_begin() to handle just that case, that being
the reason the function was written in the first place (otherwise, I
could dispose of a lot of mess, and just use an empty file as a mutex for
it).
> I do appreciate your work, but please don't commit this until the
> real issue is solved, and the dust settles. There still may be
> some use for it... plenty of time until the next release cycle.
Commit? How would I do that? :P
--
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"
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020623210636.GK81018>