Date: Mon, 19 Nov 2012 23:37:45 +0100 From: Baptiste Daroussin <bapt@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com>, Ryan Stone <rysto32@gmail.com>, FreeBSD Current <freebsd-current@freebsd.org> Subject: Re: pw keeps setting /etc/group to 0600 Message-ID: <20121119223745.GG71195@ithaqua.etoilebsd.net> In-Reply-To: <20121119222843.GB22292@dft-labs.eu> References: <CAFMmRNxDr=%2BpsiazVrJ8e=T4fogiiPv5nEAo%2BnfoD=tPYMehCw@mail.gmail.com> <CAFMmRNxW0FzupbC9w4U5pPZUoqOE%2B3rqFNRUrnsMRGFnO7qPWA@mail.gmail.com> <20121119222843.GB22292@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--+QwZB9vYiNIzNXIj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 res= ult > > 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. >=20 > My additional 0,03$: >=20 > 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. >=20 > First, pw should not fail if other instance is running, it should wait > instead (think of parallel batch scripts adding some users/groups). >=20 > Second, current code has a race: > lockfd =3D open(group_file, O_RDONLY, 0); > if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) =3D=3D -1) > err(1, "%s", group_file); > if (flock(lockfd, LOCK_EX|LOCK_NB) =3D=3D -1) { > [..] > gr_copy(pfd, tfd, gr, old_gr); /* copy from groupfile to tempfile */ > [..] > rename(tempfile,groupfile); >=20 > Now let's consider threads A and B: >=20 > A: open() > A: lock(); > A: gr_copy > B: open() >=20 > Now B has file descriptor to /etc/group that is about to be removed. >=20 > A: rename() > A: unlock() > B: lock() >=20 > Now B has a lock on unlinked file. >=20 > B: gr_copy() > B: rename() >=20 > ... and stores new content losing modifications done by A >=20 > 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 li= ke it either but at least this is consistent. regards, Bapt --+QwZB9vYiNIzNXIj Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iEYEARECAAYFAlCqtLkACgkQ8kTtMUmk6Ey/xgCdFhwC2X2HH7+SOki6QmTc7eBW sBYAn3nXdRkPnwoHVcFw61iUHdPgnNCz =lm2g -----END PGP SIGNATURE----- --+QwZB9vYiNIzNXIj--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121119223745.GG71195>