Date: Mon, 19 Nov 2012 23:28:43 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Ryan Stone <rysto32@gmail.com> Cc: bapt@freebsd.org, FreeBSD Current <freebsd-current@freebsd.org> Subject: Re: pw keeps setting /etc/group to 0600 Message-ID: <20121119222843.GB22292@dft-labs.eu> In-Reply-To: <CAFMmRNxW0FzupbC9w4U5pPZUoqOE%2B3rqFNRUrnsMRGFnO7qPWA@mail.gmail.com> References: <CAFMmRNxDr=%2BpsiazVrJ8e=T4fogiiPv5nEAo%2BnfoD=tPYMehCw@mail.gmail.com> <CAFMmRNxW0FzupbC9w4U5pPZUoqOE%2B3rqFNRUrnsMRGFnO7qPWA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Nov 17, 2012 at 11:20:21AM -0500, Ryan Stone wrote:
> My original complaint that /etc/group gets permissions of 0600 is a result
> of a bug in libutil, which bapt@ ported pw to use in r242349. The new
> group manipulation API using mktemp to create a temporary file, writes the
> new group database to the temp file and then renames the temp file to
> /etc/group. The problem here is that mktemp creates a file with a mode of
> 600, and libutil never chmods it. That should be pretty trivial to fix.
My additional 0,03$:
I took closer look to this and I think that problems are much broader
than this. I don't know if similar problems were present before.
First, pw should not fail if other instance is running, it should wait
instead (think of parallel batch scripts adding some users/groups).
Second, current code has a race:
lockfd = open(group_file, O_RDONLY, 0);
if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1)
err(1, "%s", group_file);
if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) {
[..]
gr_copy(pfd, tfd, gr, old_gr); /* copy from groupfile to tempfile */
[..]
rename(tempfile,groupfile);
Now let's consider threads A and B:
A: open()
A: lock();
A: gr_copy
B: open()
Now B has file descriptor to /etc/group that is about to be removed.
A: rename()
A: unlock()
B: lock()
Now B has a lock on unlinked file.
B: gr_copy()
B: rename()
... and stores new content losing modifications done by A
Third, I don't like current api.
gr_lock and gr_tmp have no arguments (that matter anyway)
gr_copy operates on two descriptors given as arguments
gr_mkdb takes nothing and is expected to do The Right Thing
I think descriptos should be hidden. Also I think that passing around
struct gr_something (sorry, never had talent for names) that would
contain all necessary data would be nice.
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121119222843.GB22292>