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