Date: Sun, 17 Mar 2019 10:44:47 -0600 From: Ian Lepore <ian@freebsd.org> To: Jilles Tjoelker <jilles@stack.nl>, 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: <563764a4fd41692467166a863e52e88bab030034.camel@freebsd.org> In-Reply-To: <20190317155340.GA3659@stack.nl> References: <201903160316.x2G3Gi2r072317@repo.freebsd.org> <20190317155340.GA3659@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
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. >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?563764a4fd41692467166a863e52e88bab030034.camel>