Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 20 Nov 2012 08:24:59 +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:  <20121120072459.GH71195@ithaqua.etoilebsd.net>
In-Reply-To: <20121119223745.GG71195@ithaqua.etoilebsd.net>
References:  <CAFMmRNxDr=%2BpsiazVrJ8e=T4fogiiPv5nEAo%2BnfoD=tPYMehCw@mail.gmail.com> <CAFMmRNxW0FzupbC9w4U5pPZUoqOE%2B3rqFNRUrnsMRGFnO7qPWA@mail.gmail.com> <20121119222843.GB22292@dft-labs.eu> <20121119223745.GG71195@ithaqua.etoilebsd.net>

next in thread | previous in thread | raw e-mail | index | archive | help

--poJSiGMzRSvrLGLs
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 r=
esult
> > > 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, write=
s 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 mo=
de of
> > > 600, and libutil never chmods it.  That should be pretty trivial to f=
ix.
> >=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
>=20
> gr_mkdb should chmod 0644 after renaming if rename worked.
>=20
> I should work on this soon.
>=20
> 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.
>=20
> regards,
> Bapt


Should be fixed now,

regards,
Bapt

--poJSiGMzRSvrLGLs
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)

iEYEARECAAYFAlCrMEsACgkQ8kTtMUmk6EwFPACePY6HimVwKaaDHqgRGavziX4V
KbIAnjTHaU+XBpFLvC3yNk7/YexpRr1Z
=vGGz
-----END PGP SIGNATURE-----

--poJSiGMzRSvrLGLs--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121120072459.GH71195>