daemon: explicitly allow EINTR during poll() #2002

gitgitgadget-git · 2025-06-26T00:47:32Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Carlo Marcelo Arenas Belón via GitGitGadget" <gitgitgadget@gmail.com> writes: > +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or > +# prefer to use ANSI C signal() over POSIX sigaction() > +# I may or may not have mentioned this, but this is not helpful enough for folks, as there is no clue for users to decide if they "prefer". We need to state how they need to decide between the use of "signal()" and "sigaction()" in the affected codepath, especially when they have both. If their system lacks sigaction() at all, the decision may be trivial, but the conditional compilation you are trying to achieve in daemon.c is _not_ like "my system has both, so I can use either one and the resulting binary works just fine". Rather, "Even though my platform has both, the sigaction() my system has is not quite right in such and such way and I have to use signal(), unlike everybody else, unfortunately". It may cause us to rethink the name USE_NON_POSIX_SIGNAL; I though I've already discussed this point in my response to the cover letter. Thanks.

gitgitgadget-git · 2025-06-27T01:46:05Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Carlo Marcelo Arenas Belón via GitGitGadget" <gitgitgadget@gmail.com> writes: > +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [ > + AC_COMPILE_IFELSE( > + [AC_LANG_PROGRAM([#include <signal.h>], [[ > + #ifdef SA_RESTART > + restartable signals supported > + #endif So, where SA_RESTART is defined, we fail the compilation. > + ]])],[ > + ac_cv_siginterrupt=no > + NO_RESTARTABLE_SIGNALS=UnfortunatelyYes As this is IFELSE, we know the condition that did not fail the compilation is where we did not see SA_RESTART. So we set the NO_RESTARTABLE_SIGNALS=UnfortunatelyYes, which makes sense. > + ], [ac_cv_siginterrupt=yes] > + ) > +]) > +GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS]) It is curious that throughout the two renames, the cached variable used by autoconf hasn't changed its name. Is it because it is totally invisible to the end-users/builders?

gitgitgadget-git · 2025-06-26T12:57:32Z

On the Git mailing list, Phillip Wood wrote (reply to this):

On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> > > A future change will start using sigaction to setup a SIGCHLD signal > handler. > > The current code uses signal() which returns SIG_ERR (but doesn't > seem to set errno) so instruct sigaction() to do the same. Why are we returning -1 below instead of SIG_ERR if we want the behavior to match? Thanks Phillip > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > compat/mingw-posix.h | 1 + > compat/mingw.c | 4 +++- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h > index a0dca756d104..847d558c9b2d 100644 > --- a/compat/mingw-posix.h > +++ b/compat/mingw-posix.h > @@ -95,6 +95,7 @@ struct sigaction { > sig_handler_t sa_handler; > unsigned sa_flags; > }; > +#define SA_NOCLDSTOP 1 > > struct itimerval { > struct timeval it_value, it_interval; > diff --git a/compat/mingw.c b/compat/mingw.c > index 8a9972a1ca19..5d69ae32f4b9 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -2561,7 +2561,9 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out) > > int sigaction(int sig, struct sigaction *in, struct sigaction *out) > { > - if (sig != SIGALRM) > + if (sig == SIGCHLD) > + return -1; > + else if (sig != SIGALRM) > return errno = EINVAL, > error("sigaction only implemented for SIGALRM"); > if (out)

On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this):

On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote: > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> > > > > A future change will start using sigaction to setup a SIGCHLD signal > > handler. > > > > The current code uses signal() which returns SIG_ERR (but doesn't > > seem to set errno) so instruct sigaction() to do the same. > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to > match? By "match", I mean that in both cases we will get an error return value and errno won't be set to EINVAL (which is what POSIX requires) In our codebase since we ignore the return code anyway, it wouldn't make a difference, either way. signal() returns a pointer, and sigaction() returns and int, so you can have the later be literally SIG_ERR, eventhough it will be ironically equivalent it casted into an int. Csrlo

On the Git mailing list, Phillip Wood wrote (reply to this):

On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote: > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote: >> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: >>> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> >>> >>> A future change will start using sigaction to setup a SIGCHLD signal >>> handler. >>> >>> The current code uses signal() which returns SIG_ERR (but doesn't >>> seem to set errno) so instruct sigaction() to do the same. >> >> Why are we returning -1 below instead of SIG_ERR if we want the behavior to >> match? > > By "match", I mean that in both cases we will get an error return value > and errno won't be set to EINVAL (which is what POSIX requires) > > In our codebase since we ignore the return code anyway, it wouldn't make > a difference, either way. > > signal() returns a pointer, and sigaction() returns and int, Oh right, I'd forgotten they have different return types. I think we should probably be setting errno = EINVAL before returning -1 to match what this function does with other signals it does not support - just because our current callers ignore the return value doesn't mean that future callers will and they might want check errno if they see the function fail. Thanks Phillip > so you can > have the later be literally SIG_ERR, eventhough it will be ironically > equivalent it casted into an int. > > Csrlo >

On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this):

On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote: > On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote: > > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote: > > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: > > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> > > > > > > > > A future change will start using sigaction to setup a SIGCHLD signal > > > > handler. > > > > > > > > The current code uses signal() which returns SIG_ERR (but doesn't > > > > seem to set errno) so instruct sigaction() to do the same. > > > > > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to > > > match? > > > > By "match", I mean that in both cases we will get an error return value > > and errno won't be set to EINVAL (which is what POSIX requires) > > > > In our codebase since we ignore the return code anyway, it wouldn't make > > a difference, either way. > > > > signal() returns a pointer, and sigaction() returns and int, > > Oh right, I'd forgotten they have different return types. I think we should > probably be setting errno = EINVAL before returning -1 to match what this > function does with other signals it does not support - just because our > current callers ignore the return value doesn't mean that future callers > will and they might want check errno if they see the function fail. I agree, and indeed had to triple check and change my implementation after I confirmed that signal(SIGCHLD) does not change errno on Windows (not our version, neither of the windows libc or mingw, even if it is documented[1] to do so. It might be because the signal number itself is bogus (there is none for SIGCHLD in their headers, and git uses their own numbers in compat), but either way, I would rather be consistent with signal() at least originally. Carlo [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this):

On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@gmail.com wrote: > On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote: > > On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote: > > > On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote: > > > > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote: > > > > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: > > > > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> > > > > > > > > > > > > A future change will start using sigaction to setup a SIGCHLD signal > > > > > > handler. > > > > > > > > > > > > The current code uses signal() which returns SIG_ERR (but doesn't > > > > > > seem to set errno) so instruct sigaction() to do the same. > > > > > > > > > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to > > > > > match? > > > > > > > > By "match", I mean that in both cases we will get an error return value > > > > and errno won't be set to EINVAL (which is what POSIX requires) > > > > > > > > In our codebase since we ignore the return code anyway, it wouldn't make > > > > a difference, either way. > > > > > > > > signal() returns a pointer, and sigaction() returns and int, > > > > > > Oh right, I'd forgotten they have different return types. I think we should > > > probably be setting errno = EINVAL before returning -1 to match what this > > > function does with other signals it does not support - just because our > > > current callers ignore the return value doesn't mean that future callers > > > will and they might want check errno if they see the function fail. > > > > I agree, and indeed had to triple check and change my implementation after I > > confirmed that signal(SIGCHLD) does not change errno on Windows (not our > > version, neither of the windows libc or mingw, even if it is documented[1] to > > do so. > > > > It might be because the signal number itself is bogus (there is none for > > SIGCHLD in their headers, and git uses their own numbers in compat), but > > either way, I would rather be consistent with signal() at least originally. > > I'm not sure I understand - don't we want the sigaction() wrapper to behave > like sigaction() would? for at least the first iteration, I would rather have sigaction() behave like signal(), so that the change doesn't introduce any regressions. eventually, sigaction() should behave like any other sigaction(), but to do so, I suspect the windows emulation might need to change their SIGCHLD to match. just confirmed with MSVC that if I use 20 instead of 17, errno gets updated just like the documentation says it should. Carlo PS. Maybe we should get dscho involved? > > > > [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

gitgitgadget-git · 2025-06-24T15:37:20Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Carlo Marcelo Arenas Belón via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> > > Systems without SA_RESTART where using custom CFLAGS instead of > the standard header file. This is not a sentence, isn't it? "where" -> "are"??? I dunno. > Consolidate that, so it will be easier to use in a future commit. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > compat/posix.h | 7 +++++++ > config.mak.uname | 3 --- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/compat/posix.h b/compat/posix.h > index 067a00f33b83..2612a8515897 100644 > --- a/compat/posix.h > +++ b/compat/posix.h > @@ -250,6 +250,13 @@ char *gitdirname(char *); > #define NAME_MAX 255 > #endif > > +/* On most systems <signal.h> would have given us this, but > + * not on some systems (e.g. NonStop, QNX). > + */ > +#ifndef SA_RESTART > +#define SA_RESTART 0 /* disabled for sigaction() */ > +#endif So not just on QNX and NonStop, we have SA_RESTART defined everywhere. I do not know offhand what the ramifications of this change is, but we seem to use the symbol in places outside #ifdef meaning that anybody other than QNX and NonStop that lack SA_RESTART wouldn't have been able to build Git before this change, and now they would be. The resulting binary may not work for them at all yet but that is not any worse than before. > typedef uintmax_t timestamp_t; > #define PRItime PRIuMAX > #define parse_timestamp strtoumax > diff --git a/config.mak.uname b/config.mak.uname > index b1c5c4d5e8ed..52160ef5cb07 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -654,8 +654,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > FREAD_READS_DIRECTORIES = UnfortunatelyYes > > # Not detected (nor checked for) by './configure'. > - # We don't have SA_RESTART on NonStop, unfortunalety. > - COMPAT_CFLAGS += -DSA_RESTART=0 > # Apparently needed in compat/fnmatch/fnmatch.c. > COMPAT_CFLAGS += -DHAVE_STRING_H=1 > NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease > @@ -782,7 +780,6 @@ ifeq ($(uname_S),MINGW) > endif > endif > ifeq ($(uname_S),QNX) > - COMPAT_CFLAGS += -DSA_RESTART=0 > EXPAT_NEEDS_XMLPARSE_H = YesPlease > HAVE_STRINGS_H = YesPlease > NEEDS_SOCKET = YesPlease

gitgitgadget-git · 2025-06-26T13:20:38Z

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Carlo On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> > > In a future change, the flags used for processing SIGCHLD will need to > be updated, which is only possible by using sigaction(). Only because you have chosen to use SA_RESTART here. I think it would be better to drop that and instead say something like POSIX leaves several aspects of the behavior of signal() up to the implementer. It is unspecified whether long running syscalls are restarted after an interrupt is received. The event loop in "git daemon" assumes that poll() will return with EINTR if a child exits while it is polling but if poll() is restarted after an interrupt is received that does not happen which can lead to a build up of uncollected zombie processes. It is also unspecified whether the handler is reset after an interrupt is received. In order to work correctly on operating systems such as Solaris where the handler for SIGCLD is reset when a signal is received "git daemon" calls signal() from the SIGCLD handler to re-establish a handler for that signal. Unfortunately this causes infinite recursion CEB7 on other operating systems such as AIX. Both of these problems can be addressed by using sigaction() instead of signal() which has much stricter semantics and so, by setting the appropriate flags, we can rely on poll() being interrupted and the SIGCLD handler not being reset. This change means that all long running system calls could fail with EINTR not just pall() but rest of the event loop code is designed to gracefully handle that. After this change there is still a race where a child that exits after it has been checked in check_dead_children() but before we call poll() will not be collected until a new connection is received or a child exits while we're polling. We could fix this by using the "self-pipe trick" but do not because .... Then we can drop patches 1 and 4. Best Wishes Phillip > > Replace signal() with an equivalent invocation of sigaction(), which > has the added benefit of using BSD semantics reliably and therefore > not needing the rearming call in the signal handler. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > daemon.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/daemon.c b/daemon.c > index d1be61fd5789..155b2e180167 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -915,11 +915,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) > static void child_handler(int signo UNUSED) > { > /* > - * Otherwise empty handler because systemcalls will get interrupted > - * upon signal receipt > - * SysV needs the handler to be rearmed > + * Otherwise empty handler because systemcalls should get interrupted > + * upon signal receipt. > */ > - signal(SIGCHLD, child_handler); > } > > static int set_reuse_addr(int sockfd) > @@ -1120,6 +1118,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s > > static int service_loop(struct socketlist *socklist) > { > + struct sigaction sa; > struct pollfd *pfd; > > CALLOC_ARRAY(pfd, socklist->nr); > @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist) > pfd[i].events = POLLIN; > } > > - signal(SIGCHLD, child_handler); > + sigemptyset(&sa.sa_mask); > + sa.sa_flags = SA_NOCLDSTOP | SA_RESTART; > + sa.sa_handler = child_handler; > + sigaction(SIGCHLD, &sa, NULL); > > for (;;) { > check_dead_children();

On the Git mailing list, Junio C Hamano wrote (reply to this):

Phillip Wood <phillip.wood123@gmail.com> writes: > Only because you have chosen to use SA_RESTART here. I think it would > be better to drop that and instead say something like > > > POSIX leaves several aspects of the behavior of signal() up to the > implementer. It is unspecified whether long running syscalls are > restarted after an interrupt is received. The event loop in "git > daemon" assumes that poll() will return with EINTR if a child exits > while it is polling but if poll() is restarted after an interrupt is > received that does not happen which can lead to a build up of > uncollected zombie processes. > > It is also unspecified whether the handler is reset after an interrupt > is received. In order to work correctly on operating systems such as > Solaris where the handler for SIGCLD is reset when a signal is > received "git daemon" calls signal() from the SIGCLD handler to > re-establish a handler for that signal. Unfortunately this causes > infinite recursion on other operating systems such as AIX. > > Both of these problems can be addressed by using sigaction() instead > of signal() which has much stricter semantics and so, by setting the > appropriate flags, we can rely on poll() being interrupted and the > SIGCLD handler not being reset. This change means that all long > running system calls could fail with EINTR not just pall() but rest of > the event loop code is designed to gracefully handle that. > > After this change there is still a race where a child that exits after > it has been checked in check_dead_children() but before we call poll() > will not be collected until a new connection is received or a child > exits while we're polling. We could fix this by using the "self-pipe > trick" but do not because .... > > > Then we can drop patches 1 and 4. Thanks for a suggestion. I really like the "everybody in these code paths are prepared to receive and handle EINTR just fine, so we do not have to do the SA_RESTART" observation very much.

On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this):

On Thu, Jun 26, 2025 at 08:33:29AM -0800, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > Only because you have chosen to use SA_RESTART here. I think it would > > be better to drop that and instead say something like > > > > > > POSIX leaves several aspects of the behavior of signal() up to the > > implementer. It is unspecified whether long running syscalls are > > restarted after an interrupt is received. The event loop in "git > > daemon" assumes that poll() will return with EINTR if a child exits > > while it is polling but if poll() is restarted after an interrupt is > > received that does not happen which can lead to a build up of > > uncollected zombie processes. > > > > It is also unspecified whether the handler is reset after an interrupt > > is received. In order to work correctly on operating systems such as > > Solaris where the handler for SIGCLD is reset when a signal is > > received "git daemon" calls signal() from the SIGCLD handler to > > re-establish a handler for that signal. Unfortunately this causes > > infinite recursion on other operating systems such as AIX. > > > > Both of these problems can be addressed by using sigaction() instead > > of signal() which has much stricter semantics and so, by setting the > > appropriate flags, we can rely on poll() being interrupted and the > > SIGCLD handler not being reset. This change means that all long > > running system calls could fail with EINTR not just pall() but rest of > > the event loop code is designed to gracefully handle that. > > > > After this change there is still a race where a child that exits after > > it has been checked in check_dead_children() but before we call poll() > > will not be collected until a new connection is received or a child > > exits while we're polling. We could fix this by using the "self-pipe > > trick" but do not because .... > > > > > > Then we can drop patches 1 and 4. > > Thanks for a suggestion. I really like the "everybody in these code > paths are prepared to receive and handle EINTR just fine, so we do > not have to do the SA_RESTART" observation very much. Except that is unlikely to be true, as the code has changed a lot on those 20 years (including that it now uses run_command) and that we had been effectively using SA_RESTART under the covers for most of that time because signal() behaviour changed. An obvious bug: (ex: 20250626161038.85966-1-carenas@gmail.com) I think using SA_RESTART by default might be safer, specially considering that patch 4 already exist and it is not that complicated now that we have 2. Carlo

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Carlo On 26/06/2025 17:36, Carlo Marcelo Arenas Belón wrote: > On Thu, Jun 26, 2025 at 08:33:29AM -0800, Junio C Hamano wrote: >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >> Thanks for a suggestion. I really like the "everybody in these code >> paths are prepared to receive and handle EINTR just fine, so we do >> not have to do the SA_RESTART" observation very much. > > Excep 3D11 t that is unlikely to be true, as the code has changed a lot on > those 20 years (including that it now uses run_command) and that we > had been effectively using SA_RESTART under the covers for most of > that time because signal() behaviour changed. An obvious bug: > (ex: 20250626161038.85966-1-carenas@gmail.com) I don't think the syscall handling has changed much since 695605b508 (git-daemon: Simplify dead-children reaping logic, 2008-08-14) when we started relying on poll() returning EINTR to collect children that exit while we are waiting for a new connection. In any case my comment was based on reading the code. You're right that we started using run_command() after 695605b508 but when I read the code in run-command.c it looked to me like it is pretty careful in the way it handles signals and EINTR - what part of the code are you concerned about? As git supports operating systems that do not provide SA_RESTART we should to make sure we handle EINTR correctly in this code anyway. As far as the patch you link to goes, it may not have been handling EINTR as efficiently as you'd like but it would still accept the client on the next iteration of the loop which would happen immediately because the connection would be pending when we call poll(). > I think using SA_RESTART by default might be safer, The counter argument to this is that it is masking bugs that happen on platforms without SA_RESTART. Best Wishes Phillip

gitgitgadget-git · 2025-06-26T13:26:14Z

On the Git mailing list, Phillip Wood wrote (reply to this):

On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com> > > If the setup for the SIGCHLD signal handler sets SA_RESTART, poll() > might not return with -1 and set errno to EINTR when a signal is > received. > > Since the logic to reap zombie childs relies on those interruptions > make sure to explicitly disable SA_RESTART around this function. Given "git daemon" seems to have been designed with the assumption that long running system calls can fail with EINTR I don't know why this series is trying to use SA_RESTART in the first place. Thanks Phillip > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > daemon.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/daemon.c b/daemon.c > index 155b2e180167..7e29c03e313f 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -1116,6 +1116,25 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s > } > } > > +#ifndef NO_RESTARTABLE_SIGNALS > + > +static void set_sa_restart(struct sigaction *psa, int enable) > +{ > + if (enable) > + psa->sa_flags |= SA_RESTART; > + else > + psa->sa_flags &= ~SA_RESTART; > + sigaction(SIGCHLD, psa, NULL); > +} > + > +#else > + > +static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED) > +{ > +} > + > +#endif > + > static int service_loop(struct socketlist *socklist) > { > struct sigaction sa; > @@ -1136,6 +1155,7 @@ static int service_loop(struct socketlist *socklist) > for (;;) { > check_dead_children(); > > + set_sa_restart(&sa, 0); > if (poll(pfd, socklist->nr, -1) < 0) { > if (errno != EINTR) { > logerror("Poll failed, resuming: %s", > @@ -1144,6 +1164,7 @@ static int service_loop(struct socketlist *socklist) > } > continue; > } > + set_sa_restart(&sa, 1); > > for (size_t i = 0; i < socklist->nr; i++) { > if (pfd[i].revents & POLLIN) {

-Original file line number
+Diff line change
@@ Expand Up / @@ -37,6 +37,8 @@ include shared.mak @@
     # when attempting to read from an fopen'ed directory (or even to fopen
     # it at all).
     #
+    # Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
+    #
     # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR
     # when a signal is received (as opposed to restarting).
     #
@@ Expand Down Expand Up / @@ -1811,6 +1813,9 @@ ifdef FREAD_READS_DIRECTORIES @@
     	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
     	COMPAT_OBJS += compat/fopen.o
     endif
+    ifdef NO_RESTARTABLE_SIGNALS
+    	COMPAT_CFLAGS += -DNO_RESTARTABLE_SIGNALS
+    endif
     ifdef OPEN_RETURNS_EINTR
     	COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
     endif
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -486,6 +486,7 @@ ifeq ($(uname_S),Windows) @@
     	NO_STRTOUMAX = YesPlease
     	NO_MKDTEMP = YesPlease
     	NO_INTTYPES_H = YesPlease
+    	NO_RESTARTABLE_SIGNALS = YesPlease
     	CSPRNG_METHOD = rtlgenrandom
     	# VS2015 with UCRT claims that snprintf and friends are C99 compliant,
     	# so we don't need this:
@@ Expand Down Expand Up / @@ -654,8 +655,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL) @@
     	FREAD_READS_DIRECTORIES = UnfortunatelyYes
     	# Not detected (nor checked for) by './configure'.
-    	# We don't have SA_RESTART on NonStop, unfortunalety.
-    	COMPAT_CFLAGS += -DSA_RESTART=0
     	# Apparently needed in compat/fnmatch/fnmatch.c.
     	COMPAT_CFLAGS += -DHAVE_STRING_H=1
     	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
@@ Expand All / @@ -664,6 +663,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) @@
     	NO_MMAP = YesPlease
     	NO_POLL = YesPlease
     	NO_INTPTR_T = UnfortunatelyYes
+    	NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
     	CSPRNG_METHOD = openssl
     	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
     	SHELL_PATH = /usr/coreutils/bin/bash
@@ Expand Down Expand Up / @@ -699,6 +699,7 @@ ifeq ($(uname_S),MINGW) @@
     	NEEDS_LIBICONV = YesPlease
     	NO_STRTOUMAX = YesPlease
     	NO_MKDTEMP = YesPlease
+    	NO_RESTARTABLE_SIGNALS = YesPlease
     	NO_SVN_TESTS = YesPlease
     	# The builtin FSMonitor requires Named Pipes and Threads on Windows.
@@ Expand Down Expand Up / @@ -782,7 +783,6 @@ ifeq ($(uname_S),MINGW) @@
             endif
     endif
     ifeq ($(uname_S),QNX)
-    	COMPAT_CFLAGS += -DSA_RESTART=0
     	EXPAT_NEEDS_XMLPARSE_H = YesPlease
     	HAVE_STRINGS_H = YesPlease
     	NEEDS_SOCKET = YesPlease
@@ Expand All / @@ -794,4 +794,5 @@ ifeq ($(uname_S),QNX) @@
     	NO_PTHREADS = YesPlease
     	NO_STRCASESTR = YesPlease
     	NO_STRLCPY = YesPlease
+    	NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
     endif

-Original file line number
+Diff line change
@@ Expand Up / @@ -1094,6 +1094,10 @@ else @@
       build_options_config.set('NO_EXPAT', '1')
     endif
+    if compiler.get_define('SA_RESTART', prefix: '#include <signal.h>') == ''
+      libgit_c_args += '-DNO_RESTARTABLE_SIGNALS'
+    endif
     if not compiler.has_header('sys/select.h')
       libgit_c_args += '-DNO_SYS_SELECT_H'
     endif
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

daemon: explicitly allow EINTR during poll() #2002

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -95,7 +95,7 @@ struct sigaction { @@
     	sig_handler_t sa_handler;
     	unsigned sa_flags;
     };
-    #define SA_RESTART 0
+    #define SA_NOCLDSTOP 1
     struct itimerval {
     	struct timeval it_value, it_interval;
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     int sigaction(int sig, struct sigaction *in, struct sigaction *out)
     {
-    	if (sig != SIGALRM)
+    	if (sig == SIGCHLD)
+    		return -1;
+    	else if (sig != SIGALRM)
     		return errno = EINVAL,
     			error("sigaction only implemented for SIGALRM");
     	if (out)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -250,6 +250,14 @@ char *gitdirname(char *); @@
     #define NAME_MAX 255
     #endif
+    /*
+     * On most systems <signal.h> would have given us this, but
+     * not on some systems (e.g. NonStop, QNX).
+     */
+    #ifndef SA_RESTART
+    # define SA_RESTART 0 /* disabled for sigaction() */
+    #endif
     typedef uintmax_t timestamp_t;
     #define PRItime PRIuMAX
     #define parse_timestamp strtoumax
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -862,6 +862,22 @@ fi @@
     GIT_CONF_SUBST([ICONV_OMITS_BOM])
     fi
+    # Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
+    AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
+    	AC_COMPILE_IFELSE(
+    		[AC_LANG_PROGRAM([#include <signal.h>], [[
+    			#ifdef SA_RESTART
+    			restartable signals supported
+    			#endif
+    		]])],[
+    			ac_cv_siginterrupt=no
+    			NO_RESTARTABLE_SIGNALS=UnfortunatelyYes
+    		], [ac_cv_siginterrupt=yes]
+    	)
+    ])
+    GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])
     ## Checks for typedefs, structures, and compiler characteristics.
     AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
     #
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     static void child_handler(int signo UNUSED)
     {
     	/*
-    	 * Otherwise empty handler because systemcalls will get interrupted
-    	 * upon signal receipt
-    	 * SysV needs the handler to be rearmed
+    	 * Otherwise empty handler because systemcalls should get interrupted
+    	 * upon signal receipt.
     	 */
-    	signal(SIGCHLD, child_handler);
     }
     static int set_reuse_addr(int sockfd)
@@ Expand Down Expand Up @@
     	}
     }
+    #ifndef NO_RESTARTABLE_SIGNALS
+    static void set_sa_restart(struct sigaction *psa, int enable)
+    {
+    	if (enable)
+    		psa->sa_flags |= SA_RESTART;
+    	else
+    		psa->sa_flags &= ~SA_RESTART;
+    	sigaction(SIGCHLD, psa, NULL);
+    }
+    #else
+    static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
+    {
+    }
+    #endif
     static int service_loop(struct socketlist *socklist)
     {
+    	struct sigaction sa;
     	struct pollfd *pfd;
     	CALLOC_ARRAY(pfd, socklist->nr);
@@ Expand All / @@ -1129,11 +1147,15 @@ static int service_loop(struct socketlist *socklist) @@
     		pfd[i].events = POLLIN;
     	}
-    	signal(SIGCHLD, child_handler);
+    	sigemptyset(&sa.sa_mask);
+    	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
+    	sa.sa_handler = child_handler;
+    	sigaction(SIGCHLD, &sa, NULL);
     	for (;;) {
     		check_dead_children();
+    		set_sa_restart(&sa, 0);
     		if (poll(pfd, socklist->nr, -1) < 0) {
     			if (errno != EINTR) {
     				logerror("Poll failed, resuming: %s",
@@ Expand All / @@ -1142,6 +1164,7 @@ static int service_loop(struct socketlist *socklist) @@
     			}
     			continue;
     		}
+    		set_sa_restart(&sa, 1);
     		for (size_t i = 0; i < socklist->nr; i++) {
     			if (pfd[i].revents & POLLIN) {
@@ Expand Down @@

daemon: explicitly allow EINTR during poll() #2002

Are you sure you want to change the base?

daemon: explicitly allow EINTR during poll() #2002

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!