Re: [PATCH v6 00/16] daemon-win32
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- Nov 4, 2010, 10:15 UTC
- Message-ID
- <AANLkTik=hwAmB4Ha74pqgR-cDW87GnATEpfVFDCfEHks@mail.gmail.com>
- In-Reply-To
- <alpine.DEB.2.00.1011041118200.31519@cone.home.martin.st>
On Thu, Nov 4, 2010 at 10:35 AM, Martin Storsjö <martin@martin.st> wrote:
Show 74 quoted lines
> On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
>
>> 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.
>
> 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.