gamin broken on amd64
Joe Marcus Clarke
marcus at marcuscom.com
Fri Jul 15 22:55:42 GMT 2005
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.
>
> 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
>
> struct {
> struct cmsghdr hdr;
> struct cmsgcred cred;
> } cmsg;
>
> 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.
>
> sys/socket.h says:
>
> /*
> * 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[]; */
> };
>
> 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.
>
> 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
>
> struct cmsghdr *cmsg = malloc (CMSG_SPACE(sizeof(struct cmsgcred)));
> struct cmsgcred *cred = (struct cmsgcred)CMSG_DATA(cmsg);
>
> 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:
>
> --- 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] = { 0, 0 };
> int written;
> -#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
> +#if defined(HAVE_CMSGCRED) && (!defined(LOCAL_CREDS) || defined(__FreeBSD__))
> 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
>
> retry:
> -#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
> +#if defined(HAVE_CMSGCRED) && (!defined(LOCAL_CREDS) || defined(__FreeBSD__))
> written = sendmsg(fd, &msg, 0);
> #else
> written = 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
>
> s_uid = getuid();
>
> -#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 = 1;
> @@ -139,9 +139,9 @@
> goto failed;
> }
> #ifdef HAVE_CMSGCRED
> - if (cmsg.hdr.cmsg_len < sizeof(cmsg) || cmsg.hdr.cmsg_type != SCM_CREDS) {
> + if (cmsg.hdr.cmsg_len > sizeof(cmsg) || cmsg.hdr.cmsg_type != 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
>
> 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
>
> Cheers,
> Sean
>
> _______________________________________________
> freebsd-gnome at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-gnome
> To unsubscribe, send any mail to "freebsd-gnome-unsubscribe at freebsd.org"
--
PGP Key : http://www.marcuscom.com/pgp.asc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: This is a digitally signed message part
Url : http://lists.freebsd.org/pipermail/freebsd-gnome/attachments/20050715/344d3ddc/attachment.bin
More information about the freebsd-gnome
mailing list