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

On Thu, 4 Nov 2010, Erik Faye-Lund wrote:

On Thu, Nov 4, 2010 at 9:58 AM, Martin Storsjö [off-list ref] wrote:

quoted

On Thu, 4 Nov 2010, Erik Faye-Lund wrote:

quoted

On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund [off-list ref] wrote:

quoted

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;

quoted

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