On Thu, Nov 4, 2010 at 10:35 AM, Martin Storsjà <martin@xxxxxxxxx> wrote:
> 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;
>
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 qualiïed 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
qualiïed 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html