On Mon, Nov 19, 2012 at 11:37:45PM +0100, Baptiste Daroussin wrote: > On Mon, Nov 19, 2012 at 11:28:43PM +0100, Mateusz Guzik wrote: > > 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 > > gr_mkdb should chmod 0644 after renaming if rename worked. > > I should work on this soon. > > The API has been design to match the exact same api of pw_utils, I don't like > it > either but at least this is consistent. > > regards, > Bapt
Should be fixed now, regards, Bapt
pgpiLVOJMrFr3.pgp
Description: PGP signature