Hi Vincent, On Wed, May 18, 2016 at 09:58:33AM +0200, Vincent Bernat wrote: > ??? 15 mai 2016 09:55 +0200, Vincent Bernat <ber...@luffy.cx> : > > >> I suppose that some new features of gcc started to rely on the > >> strict-aliasing rule without taking -fno-strict-aliasing into > >> consideration. I didn't find anything in the bugzilla, but it's easy to > >> miss something as there are about 500 bugs opened about aliasing. > >> > >> I'll open a bug about this. > > > > Here is the bug: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71120 > > > > With luck, we should get some pointers on why and how. > > So, it appears this is not a bug in GCC and it is not an aliasing > problem but the fact that the compiler doesn't have to use memcpy() to > copy the struct, but can copy individual member, padding excluded.
Yes, this is even a security problem which painfully affects the kernel. > And sin_addr would be in the padding, so not copied. That makes sense then. > A bug has been filed > for the glibc since the bug is in the definition: > > https://sourceware.org/bugzilla/show_bug.cgi?id=20111 Great, thanks for the link and going through this. As was mentionned there, sockaddr_storage is expected by users and in literature to be able to carry other addresses. > We could either use an union like discussed previously or locally fix > this with a memcpy(). I suppose the union would be a longer term (and > safer since we can be exhaustive about it) fix but far more invasive. I have the same feeling, but it would make a few switch/case occurences cleaner or at least more explicit. For the short term and backports I guess only memcpy() will be usable, so we should probably start by this, backport it, then work on changing all this to a union. > I suspect there are other uses of sockaddr_storage that could lead to > some bugs. If there is one, there are many. > For example, connection.c:747. ss_family will be set to 0, > but eventual port and address can be random. However, it seems that in > the remaining of the code, only ss_family is important. But maybe, it > would be better to use memset() here. Good point. Maybe we would need to develop a few macros (or inline functions) to manipulate sockaddr_storage. We already have a few, I don't think we need that many. set_port, set_addr, set_family, copy_addr and I don't know what. > On the other hand, on proto_tcp.c:225, bind_addr is correctly zeroed out > with memset. Same on standard.c:829. > > I also note on proto_http.c:3192, a "struct sockaddr_storage from" that > may be more safe to replace by "struct sockaddr_storage *from" since it > is used read-only. But maybe the compiler is smart enough to not do a > copy and therefore the bug is not triggered here. I didn't notice this one, good catch. It should even be a const. > I didn't review all uses, only uses of "struct sockaddr_storage > something". I should check for "struct sockaddr_storage *something" and > eventual use of "*something =", but I suspect that it doesn't exist > without a "struct sockaddr_storage something_else" around. I think that once we get rid of *all* sockaddr_storage we'll find them all :-) > So, quick fix is a memcpy() in cfgparse.c and in proto_http.c:3192 and > using memset() in connection.c:747 for consistency (so that somebody > doesn't copy this code for something more general). > > Can do the patches but union would be more work. That works for me. Let's have Arthur test them to confirm the issue goes away for him. Thanks! Willy
