svn commit: r345215 - projects/capsicum-test/contrib/capsicum-test

[Ian Lepore]

On Sun, 2019-03-17 at 16:53 +0100, Jilles Tjoelker wrote:
> On Sat, Mar 16, 2019 at 03:16:44AM +0000, Enji Cooper wrote:
> > Author: ngie
> > Date: Sat Mar 16 03:16:44 2019
> > New Revision: 345215
> > URL: https://svnweb.freebsd.org/changeset/base/345215
> > Log:
> >   Revert r345214
> >   It's best to not repeat the mistake I made in r300167; I should use
> >   `NO_WCAST_ALIGN` instead.
> > Modified:
> >   projects/capsicum-test/contrib/capsicum-test/capability-fd.cc
> > Modified: projects/capsicum-test/contrib/capsicum-test/capability-fd.cc
> > ==============================================================================
> > --- projects/capsicum-test/contrib/capsicum-test/capability-fd.cc	Sat Mar 16 03:03:25 2019	(r345214)
> > +++ projects/capsicum-test/contrib/capsicum-test/capability-fd.cc	Sat Mar 16 03:16:44 2019	(r345215)
> > @@ -982,14 +982,12 @@ FORK_TEST_ON(Capability, SocketTransfer, TmpFile("cap_
> >      // Child: enter cap mode
> >      EXPECT_OK(cap_enter());
> >  
> > -    int cap_fd;
> > -
> >      // Child: wait to receive FD over socket
> >      int rc = recvmsg(sock_fds[0], &mh, 0);
> >      EXPECT_OK(rc);
> >      EXPECT_LE(CMSG_LEN(sizeof(int)), mh.msg_controllen);
> >      cmptr = CMSG_FIRSTHDR(&mh);
> > -    memcpy(&cap_fd, CMSG_DATA(cmptr), sizeof(int));
> > +    int cap_fd = *(int*)CMSG_DATA(cmptr);
> >      EXPECT_EQ(CMSG_LEN(sizeof(int)), cmptr->cmsg_len);
> >      cmptr = CMSG_NXTHDR(&mh, cmptr);
> >      EXPECT_TRUE(cmptr == NULL);
> > @@ -1024,7 +1022,7 @@ FORK_TEST_ON(Capability, SocketTransfer, TmpFile("cap_
> >    cmptr->cmsg_level = SOL_SOCKET;
> >    cmptr->cmsg_type = SCM_RIGHTS;
> >    cmptr->cmsg_len = CMSG_LEN(sizeof(int));
> > -  memcpy(CMSG_DATA(cmptr), &cap_fd, sizeof(int));
> > +  *(int *)CMSG_DATA(cmptr) = cap_fd;
> >    buffer1[0] = 0;
> >    iov[0].iov_len = 1;
> >    sleep(3);
> 
> I think it is better to suppress the alignment warning via an
> intermediate (void *) cast than to suppress it more globally.
> 
> As for other considerations between dereferencing cast pointers and
> memcpy(), the use of memcpy() also allows reading and writing regardless
> of the effective type of the object (strict aliasing). However, that is
> of limited use in this particular case since the CMSG_NXTHDR macro
> already dereferences a cast pointer -- the buffer pointed to by
> msg_control must have a suitable effective type.
> 
> In this case, the buffer pointed to by msg_control is a char array,
> which may only be accessed using lvalues of type char, signed char,
> unsigned char or qualified versions of those. Additionally, it is not,
> in general, guaranteed to be suitably aligned (the amd64 ABI guarantees
> it, though).
> 

I believe it is suitably aligned.  When these values are constructed in
the kernel they use the _ALIGN() macro which is defined in
machine/_align.h and is supposed to give the appropriate alignment for
the platform for any type that can appear in the control-message area.

-- Ian

> The obviously correct fix is to allocate the buffer using calloc(),
> which provides suitably aligned memory without an effective type. Note
> that allocating a char array on the heap using C++ new would probably
> not help since I expect such memory to have an effective type already.
> If making the program less efficient to make it correct regarding
> strict-aliasing is undesirable, it may be possible to use trickery with
> a union such as in lib/libopenbsd/imsg.c, but I don't know whether that
> is actually correct. (Dividing things between translation units may also
> hide declared types and force the compiler to assume that the effective
> type is suitable, but I don't immediately know a way to do that without
> making things really messy.)
> 
> By the way, the use of memcpy() removed here looks correct to me.
> 
> On another note, ((char *)(cmsg) + _ALIGN(((struct cmsghdr
> *)(cmsg))->cmsg_len) + _ALIGN(sizeof(struct cmsghdr)) > (char
> *)(mhdr)->msg_control + (mhdr)->msg_controllen) in <sys/socket.h>'s
> CMSG_NXTHDR looks wrong since it calculates a pointer which is out of
> range. It would be better to calculate the integer ((char *)(cmsg) -
> (char *)(mhdr)) first and do everything else using integer arithmetic. I
> may submit a review later.
>