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

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

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

quoted

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;

Yes, at least with C99. Section 6.5, paragraph 7 of the C99 specification says:

"An object shall have its stored value accessed only by an lvalue
expression that has one of
the following types:

- a type compatible with the effective type of the object,
- a qualified version of a type compatible with the effective type of the object,
- a type that is the signed or unsigned type corresponding to the
effective type of the
object,
- a type that is the signed or unsigned type corresponding to a
qualified version of the
effective type of the object,
- an aggregate or union type that includes one of the aforementioned
types among its
members (including, recursively,amember of a subaggregate or contained
union), or
- a character type."

A type punned pointer does not meet any of those requirements.

For pre-C99 I don't have any reference other than Wikpedia, which
indicates that it's illegal in the ISO version of the standard (which
I assume must be C90):

"To enable such optimizations in a predictable manner, the ISO
standard for the C programming language (including its newer C99
edition, see section 6.5, paragraph 7) specifies that it is illegal
(with some exceptions) for pointers of different types to reference
the same memory location."

ref: http://en.wikipedia.org/wiki/Aliasing_(computing)

But the code above is incorrect in another sense: it's declaring a
symbol with a name starting with double underscore, something that is
reserved for the compiler. So I don't think I would trust the person
who wrote it to care much about standards compliance.