git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Erik Faye-Lund <kusmabite@gmail.com>
Date
Nov 4, 2010, 09:15 UTC
Message-ID
<AANLkTimzS1tnykzjoywaLdBUi4zhwzUC6bght0-vnaGO@mail.gmail.com>
In-Reply-To
<alpine.DEB.2.00.1011041053120.31519@cone.home.martin.st>
On Thu, Nov 4, 2010 at 9:58 AM, Martin Storsjö <martin@martin.st> wrote:
Show 93 quoted lines
> 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:
>> > On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> >> On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> >>> On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
>> >>> <patthoyts@users.sourceforge.net> wrote:
>> >>>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>> >>>>
>> >>>>>Here's hopefully the last iteration of this series. The previous version
>> >>>>>only got a single complain about a typo in the subject of patch 14/15, so
>> >>>>>it seems like most controversies have been settled.
>> >>>>
>> >>>> I pulled this win32-daemon branch into my msysgit build tree and built
>> >>>> it. I get the following warnings:
>> >>>>
>> >>>>    CC daemon.o
>> >>>> daemon.c: In function 'service_loop':
>> >>>> daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
>> >>>> daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
>> >>>> daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
>> >>>> daemon.c:919: note: initialized from here
>> >>>> daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
>> >>>> daemon.c:675: note: initialized from here
>> >>>> daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
>> >>>> daemon.c:682: note: initialized from here
>> >>>>
>> >>>
>> >>> Yeah, I'm aware of these. I thought those warnings were already
>> >>> present in the Linux build, but checking again I see that that's not
>> >>> the case. Need to investigate.
>> >>>
>> >>
>> >> OK, it's the patch "daemon: use run-command api for async serving"
>> >> that introduce the warning. But looking closer at the patch it doesn't
>> >> seem the patch actually introduce the strict-aliasing violation, it's
>> >> there already. The patch only seems to change the code enough for GCC
>> >> to start realize there's a problem. Unless I'm misunderstanding
>> >> something vital, that is.
>> >>
>> >> Anyway, here's a patch that makes it go away, I guess I'll squash it
>> >> into the next round.
>> >>
>> >
>> > 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.

Show 21 quoted lines
> FWIW, the actual warning itself isn't directly related to any of the code
> worked on here, gcc just happens to realize it after some of these
> changes. I'm able to trigger the same warnings on the current master, by
> simply doing this change:
>
> diff --git a/daemon.c b/daemon.c
> index 5783e24..467cea2 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -670,7 +670,6 @@ static void handle(int incoming, struct sockaddr
> *addr, int
>        dup2(incoming, 1);
>        close(incoming);
>
> -       exit(execute(addr));
>  }
>
>  static void child_handler(int signo)
>
>
> // Martin
Yeah, I already figured that one out, but thanks for making it clearer. :)

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.

Previous: Martin StorsjöNext: Martin Storsjö
Message 28 of 31 in “daemon-win32”
  1. 00/16 daemon-win32Erik Faye-Lund, Nov 3, 2010
  2. 01/16 mingw: add network-wrappers for daemonErik Faye-Lund, Nov 3, 2010
  3. 02/16 mingw: implement syslogErik Faye-Lund, Nov 3, 2010
  4. 03/16 compat: add inet_pton and inet_ntop prototypesErik Faye-Lund, Nov 3, 2010
  5. 04/16 inet_ntop: fix a couple of old-style declsErik Faye-Lund, Nov 3, 2010
  6. 05/16 mingw: use real pidErik Faye-Lund, Nov 3, 2010
  7. 06/16 mingw: support waitpid with pid > 0 and WNOHANGErik Faye-Lund, Nov 3, 2010
  8. 07/16 mingw: add kill emulationErik Faye-Lund, Nov 3, 2010
  9. 08/16 daemon: use run-command api for async servingErik Faye-Lund, Nov 3, 2010
  10. 09/16 daemon: use full buffered mode for stderrErik Faye-Lund, Nov 3, 2010
  11. 10/16 Improve the mingw getaddrinfo stub to handle more use casesErik Faye-Lund, Nov 3, 2010
  12. 11/16 daemon: get remote host address from root-processErik Faye-Lund, Nov 3, 2010
  13. 12/16 mingw: import poll-emulation from gnulibErik Faye-Lund, Nov 3, 2010
  14. 13/16 mingw: use poll-emulation from gnulibErik Faye-Lund, Nov 3, 2010
  15. 14/16 daemon: use socklen_tErik Faye-Lund, Nov 3, 2010
  16. 15/16 daemon: make --inetd and --detach incompatibleErik Faye-Lund, Nov 3, 2010
  17. 16/16 daemon: opt-out on features that require posixErik Faye-Lund, Nov 3, 2010
  18. Pat ThoytsNov 3, 2010
  19. Erik Faye-LundNov 3, 2010
  20. Erik Faye-LundNov 3, 2010
  21. Pat ThoytsNov 4, 2010
  22. Erik Faye-LundNov 4, 2010
  23. Pat ThoytsNov 4, 2010
  24. Erik Faye-LundNov 3, 2010
  25. Erik Faye-LundNov 4, 2010
  26. Erik Faye-LundNov 4, 2010
  27. Martin StorsjöNov 4, 2010
  28. Erik Faye-LundNov 4, 2010
  29. Martin StorsjöNov 4, 2010
  30. Erik Faye-LundNov 4, 2010
  31. Martin StorsjöNov 4, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.