Re: [PATCH v6 00/16] daemon-win32

> On Thu, Nov 4, 2010 at 9:58 AM, Martin Storsjö <martin@martin.st> wrote:
> > On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
> >
> >> On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund <kusmabite@gmail.com> 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.

> > 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.