From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 74241 invoked by alias); 9 Mar 2015 11:10:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 74226 invoked by uid 89); 9 Mar 2015 11:10:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 09 Mar 2015 11:10:25 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t29BAN02006799 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 9 Mar 2015 07:10:23 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t29BALI4014316; Mon, 9 Mar 2015 07:10:22 -0400 Message-ID: <54FD7F9D.9070809@redhat.com> Date: Mon, 09 Mar 2015 11:10:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Eli Zaretskii CC: gdb-patches@sourceware.org Subject: Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations References: <1425750266-14385-1-git-send-email-palves@redhat.com> <83r3t0lmb9.fsf@gnu.org> <54FB4162.5090601@redhat.com> <83oao4ljwg.fsf@gnu.org> <54FD79B4.8070201@redhat.com> In-Reply-To: <54FD79B4.8070201@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00213.txt.bz2 On 03/09/2015 10:45 AM, Pedro Alves wrote: > On 03/07/2015 06:52 PM, Eli Zaretskii wrote: >>> Date: Sat, 07 Mar 2015 18:20:18 +0000 >>> From: Pedro Alves >>> CC: gdb-patches@sourceware.org >>> >>> Those are BSD socket types, they've been this way ever since BSD >>> invented them. The structs are not type compatible, even though >>> they have some common fields that are are put at the same offsets, >>> by design. >>> >>> See e.g.: >>> >>> http://stackoverflow.com/questions/1429645/how-to-cast-sockaddr-storage-and-avoid-breaking-strict-aliasing-rules >> >> IMO, the right way to handle this is to have our own struct (NOT >> union!) with the data, and fill the correct struct from it, field by >> field, when we need to pass it to a library function. > > But that doesn't really make sense in this context. > These struct sockaddr_foo types aren't ours, they're pretty much > hard core to the sockets library. You _need_ to fill in a > struct sockaddr_foo, and pass that to bind/accept, but disguised > as a "struct sockaddr". That's just how those calls work. It's > impossible to fill in a struct sockaddr with the data, because > struct sockaddr does not have the necessary fields. E.g.: > > (top-gdb) p sizeof (struct sockaddr) > $3 = 16 > (top-gdb) p sizeof (struct sockaddr_in) > $2 = 16 > (top-gdb) p sizeof (struct sockaddr_in6) > $1 = 28 > (top-gdb) p sizeof (struct sockaddr_un) > $4 = 110 > > if 'struct sockaddr' HAD been defined like: > > struct sockaddr { > sa_family_t sa_family; > } > > and then struct sockaddr_foos had been defined as "extending" > struct sockaddr, like e.g.: > > struct sockaddr_in6 { > /* Extend a sockaddr */ > struct sockaddr sa; > > /* ... and add more data */ > in_port_t sin6_port; > uint32_t sin6_flowinfo; > struct in6_addr sin6_addr; > uint32_t sin6_scope_id; > } > > That is, the first field of the structure had been the > abstract "struct sockaddr", then it'd be OK to cast struct sockaddr_foo > to "struct sockaddr". C allows that. > > (Another possibility would have been for bind/accept to > take a "sa_family_t *sa_family" as argument instead of > "struct sockaddr *", same thing.) > > But, they're not. That ship has sailed many years ago. > What we have is types like these (from gdb's 'ptype', > typedefs may be missing): > > struct sockaddr { > sa_family_t sa_family; > char sa_data[14]; > } > > struct sockaddr_in { > sa_family_t sin_family; > in_port_t sin_port; > struct in_addr sin_addr; > unsigned char sin_zero[8]; > } > > struct sockaddr_in6 { > sa_family_t sin6_family; > in_port_t sin6_port; > uint32_t sin6_flowinfo; > struct in6_addr sin6_addr; > uint32_t sin6_scope_id; > } > > struct sockaddr_un { > sa_family_t sun_family; > char sun_path[108]; > } > > And thus casting between "struct sockaddr *" > and "struct sockaddr_in6 *" is invalid. These types can't > alias, thus with this: > > struct sockaddr_in *addr_in = ...; > struct sockaddr *addr = (struct sockaddr *) addr_in; > > addr->sa_family = 1; > addr_in->sin_family = 2; > if (addr->sa_family != 1) { > abort (); > } > > the compiler is free to completely eliminate the 'if'. > >> A union is just a type-cast in disguise; if using it is wrong, we >> shouldn't try. (And what if one of these days GCC will acquire a >> capability to see through them?) >> > > As discussed in that stackexchange page, it won't. It's > implementation-defined till C90 whether accessing a different union member > from the one last used to store a value was valid, a.k.a., type-punning. > > In GCC's implementation (and others' I believe), it's valid. > Please see: > > https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation > and: > https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Optimize-Options.html#Type-punning > > And then the C99 standard changed its wording to make it valid everywhere. > > See DR283 (defect report): > http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm > > And C99 TC3, section 6.5, paragraph 7: > www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf > > Casts are invalid. Unions work in practice with old compilers, > and are required to work in "modern" (old by now) compilers. So > really unions are the proper solution for this, however unfortunate. > >>> It's not a C++ restriction. The old code was invalid C code. >> >> I'm talking about the new code. > > Then I don't understand what you're saying, sorry. This has nothing > to do with C++. C++ happened to catch it due to the missing cast. I > could have added the cast to that one place that missed it, but that > would just be propagating both invalid C++ _and_ invalid _C_ code. With all that said, I'm having second thoughts on this ... Per 6.3.2.3 ("A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type"), as long as we don't actually access the fields of sockaddr through the "struct sockaddr *" pointer, then the cast should be OK. In the bind/accept cast, it should be the internals of those functions that need the union trick. Gosh, what a mess. So I'll revert the union patch, and apply the original one that added the missing cast to tracepoint.c... Thanks, Pedro Alves