On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
> On Thu, Nov 4, 2010 at 9:58 AM, Martin Storsjö <martin@xxxxxxxxx> wrote:
> > On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
> >
> >> On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote:
> >> >
> >> > Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
> >> > with IPv6 support) in a union and passing that around instead does
> >> > seem to fix the issue completely. I don't find it very elegant, but
> >> > some google-searches on the issue seems to reveal that this is the
> >> > only way of getting rid of this. Any other suggestions, people?
> >> >
> >>
> >> Just for reference, this is the patch that fixes it. What do you think?
> >>
> >> diff --git a/daemon.c b/daemon.c
> >> index 941c095..8162f10 100644
> >> --- a/daemon.c
> >> +++ b/daemon.c
> >> @@ -902,9 +903,15 @@ static int service_loop(struct socketlist *socklist)
> >>
> >> for (i = 0; i < socklist->nr; i++) {
> >> if (pfd[i].revents & POLLIN) {
> >> - struct sockaddr_storage ss;
> >> + union {
> >> + struct sockaddr sa;
> >> + struct sockaddr_in sai;
> >> +#ifndef NO_IPV6
> >> + struct sockaddr_in6 sai6;
> >> +#endif
> >> + } ss;
> >> unsigned int sslen = sizeof(ss);
> >> - int incoming = accept(pfd[i].fd, (struct sockaddr *)&ss, &sslen);
> >> + int incoming = accept(pfd[i].fd, &ss.sa, &sslen);
> >> if (incoming < 0) {
> >> switch (errno) {
> >> case EAGAIN:
> >> @@ -915,7 +922,7 @@ static int service_loop(struct socketlist *socklist)
> >> die_errno("accept returned");
> >> }
> >> }
> >> - handle(incoming, (struct sockaddr *)&ss, sslen);
> >> + handle(incoming, &ss.sa, sslen);
> >> }
> >> }
> >> }
> >
> > As you say yourself, it's not elegant at all - sockaddr_storage is
> > intended to be just that, an struct large enough to fit all the sockaddrs
> > you'll encounter on this platform, with all fields aligned in the same way
> > as all the other sockaddr structs. You're supposed to be able to cast the
> > sockaddr struct pointers like currently is done, although I'm not familiar
> > with the strict aliasing stuff well enough to know if anything else would
> > be required somewhere.
> >
>
> Strict aliasing isn't exactly about the structure being large enough
> or not, it's only being able to access a particular piece of memory
> through one type only (unless specificly marked with "union").
> sockaddr_storage is an attempt at fixing the storage-problem without
> addressing the type punning problem, which doesn't help us much.
Given this, does that mean that all code that uses sockaddr_storage
directly without a union, and casting pointers to this struct into
sockaddr, sockaddr_in and sockaddr_in6 is incorrect with regards to strict
aliasing?
That is, even this example from RFC 2553 is faulty:
struct sockaddr_storage __ss;
struct sockaddr_in6 *sin6;
sin6 = (struct sockaddr_in6 *) &__ss;
> > I didn't see any of these hacks in the v7 patchset - did the warning go
> > away by itself there?
> >
>
> The strict aliasing problem was (as I described earlier, and you
> pointed out later in your email) already present in the code. All the
> patch did, was modify the code enough to make GCC realize it.
>
> Since the code is removed by a later patch ("daemon: get remote host
> address from root-process"), I figured adding a union just to remove
> it was just noisy. So instead I changed the code enough for the
> warning to go away again. It turned out that it was the assignment of
> NULL to "peer" that triggered the warning, so I made two calls to
> execute() instead, one that pass NULL and one that pass "peer".
>
> Then in "daemon: get remote host address from root-process" which
> causes a similar strict-aliasing issue (this time I'm the one who
> introduced it, since the handle() code-path doesn't derefence "addr"),
> I fixed it with a union.
Ah, I didn't see that one. Ok, that explains it.
The union solution is correct although not pretty, while the one changing
it to a bare sockaddr was wrong, which was what caught my attention. :-)
> So a nice end-result of v7 is that we're good with strict aliasing,
> which means that it's not safe(er) to compile git-daemon on GCC with
> -O3.
Actually, couldn't there still be similar strict aliasing violations left
that GCC hasn't realized yet?
// Martin