From owner-svn-src-projects@freebsd.org Sun Mar 17 15:53:45 2019 Return-Path: Delivered-To: svn-src-projects@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6A064154016A for ; Sun, 17 Mar 2019 15:53:45 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from ecc03.stack.nl (ecc03.stack.nl [IPv6:2001:610:1108:5010::210]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.stack.nl", Issuer "COMODO RSA Domain Validation Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AEE5B8565C; Sun, 17 Mar 2019 15:53:44 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mail02.stack.nl (blade.stack.nl [51.15.111.152]) by ecc03.stack.nl (Postfix) with ESMTPS id D82D4202BC; Sun, 17 Mar 2019 15:53:41 +0000 (UTC) Received: from localhost (localhost.localdomain [127.0.0.1]) by mail02.stack.nl (Postfix) with ESMTP id C0D51241798; Sun, 17 Mar 2019 15:53:41 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at mail02 Received: from mail02.stack.nl ([127.0.0.1]) by localhost (mail02.stack.nl [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id wO9SrX5quG3M; Sun, 17 Mar 2019 15:53:40 +0000 (UTC) Received: from blade.stack.nl (blade.stack.nl [192.168.122.130]) by mail02.stack.nl (Postfix) with ESMTP id 3B321241792; Sun, 17 Mar 2019 15:53:40 +0000 (UTC) Received: by blade.stack.nl (Postfix, from userid 1677) id 292C42057D; Sun, 17 Mar 2019 16:53:40 +0100 (CET) Date: Sun, 17 Mar 2019 16:53:40 +0100 From: Jilles Tjoelker To: Enji Cooper 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> References: <201903160316.x2G3Gi2r072317@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201903160316.x2G3Gi2r072317@repo.freebsd.org> User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Mar 2019 15:53:45 -0000 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 '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