Date: Sun, 17 Mar 2019 16:53:40 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: Enji Cooper <ngie@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r345215 - projects/capsicum-test/contrib/capsicum-test Message-ID: <20190317155340.GA3659@stack.nl> In-Reply-To: <201903160316.x2G3Gi2r072317@repo.freebsd.org> References: <201903160316.x2G3Gi2r072317@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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). 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. -- Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190317155340.GA3659>