Date: Fri, 15 Jul 2005 18:55:35 -0400 From: Joe Marcus Clarke <marcus@marcuscom.com> To: sean@mcneil.com Cc: gnome@freebsd.org Subject: Re: gamin broken on amd64 Message-ID: <1121468135.56153.5.camel@shumai.marcuscom.com> In-Reply-To: <1121419728.4388.31.camel@server.mcneil.com> References: <1121419728.4388.31.camel@server.mcneil.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-Y+UfBVub7ovoavKjM2kT
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable
On Fri, 2005-07-15 at 02:28 -0700, Sean McNeil wrote:
> msg credentials are difficult to get right in a portable fashion. I'm
> not so certain that FreeBSD is doing things right here, but I have a
> patch to resolve the issue.
>=20
> Typically, one would create a structure that includes the cmsghdr + the
> actual message going across. server/gam_channel.c is a classic example.
> It defines a cmsg variable as
>=20
> struct {
> struct cmsghdr hdr;
> struct cmsgcred cred;
> } cmsg;
>=20
> Even though the documentation says that the sender is suppose to reserve
> the space in msg.msg_control, it appears that FreeBSD will allocate its
> own space and fill it in. When it does so, it uses the CMSG_DATA() to
> get the start of what should be struct cmsgcred cred. But it aligns
> based on the _ALIGN() macro. For AMD64, this is 8 bytes. But this
> means that when read on the other end, the credentials are all hosed up
> because they are off by 4 bytes.
>=20
> sys/socket.h says:
>=20
> /*
> * Header for ancillary data objects in msg_control buffer.
> * Used for additional information with/about a datagram
> * not expressible by flags. The format is a sequence
> * of message elements headed by cmsghdr structures.
> */
> struct cmsghdr {
> socklen_t cmsg_len; /* data byte count, including hdr */
> int cmsg_level; /* originating protocol */
> int cmsg_type; /* protocol-specific type */
> /* followed by u_char cmsg_data[]; */
> };
>=20
> technically, this is not true. cmsg_data[] is really adjusted to the
> alignment of the machine. Makes sense, I guess, since you probably want
> the data starting in the same place regardless of whether it has any
> long variables in it.
>=20
> Further, if there are any long values, then you would want to make sure
> that msg.msg_control is pointing to a properly aligned memory location.
> None of the examples I've seen take this into account. This means the
> safest way to do things would be to have something like
>=20
> struct cmsghdr *cmsg =3D malloc (CMSG_SPACE(sizeof(struct cmsgcred)));
> struct cmsgcred *cred =3D (struct cmsgcred)CMSG_DATA(cmsg);
>=20
> This, IMHO, is the most portable and "proper" way to accomplish things.
> But I just wanted a cheap and dirty solution. To solve this easily, I
> added an alignment attribute and changed a verification test. I created
> a new patch-server_gam_channel.c:
>=20
> --- server/gam_channel.c.orig Fri Jul 15 01:13:44 2005
> +++ server/gam_channel.c Fri Jul 15 01:50:54 2005
> @@ -29,10 +29,10 @@
> {
> char data[2] =3D { 0, 0 };
> int written;
> -#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
> +#if defined(HAVE_CMSGCRED) && (!defined(LOCAL_CREDS) || defined(__FreeBS=
D__))
> struct {
> struct cmsghdr hdr;
> - struct cmsgcred cred;
> + struct cmsgcred cred __attribute__ ((aligned (_ALIGN(1))));
> } cmsg;
> struct iovec iov;
> struct msghdr msg;
> @@ -53,7 +53,7 @@
> #endif
> =20
> retry:
> -#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
> +#if defined(HAVE_CMSGCRED) && (!defined(LOCAL_CREDS) || defined(__FreeBS=
D__))
> written =3D sendmsg(fd, &msg, 0);
> #else
> written =3D write(fd, &data[0], 1);
> @@ -94,13 +94,13 @@
> #ifdef HAVE_CMSGCRED
> struct {
> struct cmsghdr hdr;
> - struct cmsgcred cred;
> + struct cmsgcred cred __attribute__ ((aligned (_ALIGN(1))));
> } cmsg;
> #endif
> =20
> s_uid =3D getuid();
> =20
> -#if defined(LOCAL_CREDS) && defined(HAVE_CMSGCRED)
> +#if defined(LOCAL_CREDS) && defined(HAVE_CMSGCRED) && !defined(__FreeBSD=
__)
> /* Set the socket to receive credentials on the next message */
> {
> int on =3D 1;
> @@ -139,9 +139,9 @@
> goto failed;
> }
> #ifdef HAVE_CMSGCRED
> - if (cmsg.hdr.cmsg_len < sizeof(cmsg) || cmsg.hdr.cmsg_type !=3D SCM_=
CREDS) {
> + if (cmsg.hdr.cmsg_len > sizeof(cmsg) || cmsg.hdr.cmsg_type !=3D SCM_=
CREDS) {
> GAM_DEBUG(DEBUG_INFO,
> - "Message from recvmsg() was not SCM_CREDS\n");
> + "Message from recvmsg() was not SCM_CREDS.\n");
> goto failed;
> }
> #endif
>=20
> This got me working again on AMD64. I've also attached it to assure
> proper spacing and such. If I ever get enough time, I can whip up a
> "correct" patch for this. By the way, the gnome keyring manager has
> this same issue, but the fortunate thing is that looking at the
> effective uid gives you the uid, which is usually the same. gamin
> croaks because the pid ends up being 0 from the 4-byte offset.
I see what you're doing in two of the hunks, but why are you changing
the comparison of cmsg.hdr.cmsg_len to sizeof(cmsg)? I think the way it
was is correct.
In any event, I'd rather see the right fix for this go in since it
affects gnome-keyring, dbus, and gamin. I'll work on it over the
weekend.
Joe
>=20
> Cheers,
> Sean
>=20
> _______________________________________________
> freebsd-gnome@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-gnome
> To unsubscribe, send any mail to "freebsd-gnome-unsubscribe@freebsd.org"
--=20
PGP Key : http://www.marcuscom.com/pgp.asc
--=-Y+UfBVub7ovoavKjM2kT
Content-Type: application/pgp-signature; name=signature.asc
Content-Description: This is a digitally signed message part
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (FreeBSD)
iD8DBQBC2D7nb2iPiv4Uz4cRAiXwAKCj/Fb9Qp/qKRCwQ6h2tNevGOGm6gCfTPb+
2voknBcFmagogy5scIvw0O4=
=frHE
-----END PGP SIGNATURE-----
--=-Y+UfBVub7ovoavKjM2kT--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1121468135.56153.5.camel>