From owner-svn-src-projects@freebsd.org Sun Mar 17 16:45:02 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 EE88B1541DF2 for ; Sun, 17 Mar 2019 16:45:01 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from outbound1.eu.mailhop.org (outbound1.eu.mailhop.org [52.28.251.132]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 673708707F for ; Sun, 17 Mar 2019 16:45:01 +0000 (UTC) (envelope-from ian@freebsd.org) ARC-Seal: i=1; a=rsa-sha256; t=1552841092; cv=none; d=outbound.mailhop.org; s=arc-outbound20181012; b=fwkA4wS1uaxz4jo4a4PvKFKh/3XKCphp5pQbcjgNgnXK8fMtSvxiyLBS7Su/lR6y5rjgRDxYu+aBL hqtW903RN805BtHRJyuWcwTxOsHjx8v8lS6QWchc6V8sccoXSIv/oGxZsjhCIKc6j35gqRSLCSzbEk QiAVcuOMhWMKaC8E0G9fzyPz+eJmU3A1H8/C908fFH/KszdkgeoYN5LRXI+43vJyuV3287lvMsAI5x iTvUhwvmpi3BCrOr0LTynrvausB7xL7Kz1jMNaCQ8Bxr3hBAUHCabTuwxw5QUmWT5JT4r0d4KvTpLR 63XTxA73KmaPmwn+62sFRo8CwIoK1hw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=arc-outbound20181012; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:cc:to:from:subject:message-id:dkim-signature:from; bh=AkrUTaO6m4QdF+9qcAWgCv5SKRt9yG/kugON2ShuH/Y=; b=NiOoQryOjjZlRNxO/POo88xFr1G5SRzC+euSPBBsQNw/deSCF73D3er/c1F/4ENRsqhtsgN38LBNy f7TY+6A9KQ1r7qpnmlf4ortPsrulLUx/MMgSW+hjuaGfY1qHjocPZWxz4Fjb+PNov9U4mcgr6WOvAB deIGVMS0GExzWAdTWqa63AjdOsqc1DERcaVAwVZGHx92WhK7W+ZZw73xGf4w6TvjNdu6AF33NvjS9O yweq698qKarB1cTo0P5M+a4TP+ei1Au71igwd27FNdwhAwo/AGElZ2a93k20FaWpD2MocAm1vl+5cA M/a11SXIYXwv0j8imj4YaiNjD/QVctg== ARC-Authentication-Results: i=1; outbound3.eu.mailhop.org; spf=softfail smtp.mailfrom=freebsd.org smtp.remote-ip=67.177.211.60; dmarc=none header.from=freebsd.org; arc=none header.oldest-pass=0; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=dkim-high; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:cc:to:from:subject:message-id:from; bh=AkrUTaO6m4QdF+9qcAWgCv5SKRt9yG/kugON2ShuH/Y=; b=ETzpN1x+ApuUhWxySgPD5/NAcsM+DoceFi7uQm2rbS+lDLA/l0XNMVda6URtzeVaQ+I4/M/tONO/m DolD9X90FhTxWyeS2/Ft1BpBqE5UkcISstcbYTGdwyAmgegdL/i7WZzH5807StLQvmqTLQHWTpiE0o a3l1uPTo93qJw8U/EnglALj1dYg4JUGA8EPJKsq/fV3/dhAyKkreMRahReswEHoEfYZxLeEeHmM3jU GOuK8ocJQ6t3fMvqrWI6JgmUoUoa17QN5EpzrPBVRAd5kcYw2CL1xRiCN2qbbbI85+LipTIfmWIUt6 /XT00e1kXheN7PAjv1F99k3DxKCKkLw== X-MHO-RoutePath: aGlwcGll X-MHO-User: fa1dc8c6-48d3-11e9-908b-352056dbf2de X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [67.177.211.60]) by outbound3.eu.mailhop.org (Halon) with ESMTPSA id fa1dc8c6-48d3-11e9-908b-352056dbf2de; Sun, 17 Mar 2019 16:44:50 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id x2HGilHj059711; Sun, 17 Mar 2019 10:44:48 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <563764a4fd41692467166a863e52e88bab030034.camel@freebsd.org> Subject: Re: svn commit: r345215 - projects/capsicum-test/contrib/capsicum-test From: Ian Lepore To: Jilles Tjoelker , Enji Cooper Cc: src-committers@freebsd.org, svn-src-projects@freebsd.org Date: Sun, 17 Mar 2019 10:44:47 -0600 In-Reply-To: <20190317155340.GA3659@stack.nl> References: <201903160316.x2G3Gi2r072317@repo.freebsd.org> <20190317155340.GA3659@stack.nl> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 FreeBSD GNOME Team Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 673708707F X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.99 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.99)[-0.989,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] 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 16:45:02 -0000 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 '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. >