Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

On Sat, Jan 9, 2010 at 1:49 AM, Erik Faye-Lund [off-list ref] wrote:

On Wed, Dec 2, 2009 at 8:27 PM, Johannes Sixt [off-list ref] wrote:

quoted

On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:

quoted

On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt [off-list ref] wrote:

quoted

"relatively small chance of stuff blowing up"? The docs of
TerminateThread: "... the kernel32 state for the thread's process could
be inconsistent." That's scary if we are talking about a process that
should run for days or weeks without interruption.

I think there's a misunderstanding here. I thought your suggestion was
to simply call die(), which would take down the main process. After
reading this explanation, I think you're talking about giving an error
and rejecting the connection instead. Which makes more sense than to
risk crashing the main-process, indeed.

Just rejecting a connection is certainly the simplest do to keep the daemon
process alive. But the server can be DoS-ed from a single source IP.

Currently git-daemon can only be DDoS-ed because there is a maximum number of
connections, which are not closed if all of them originate from different
IPs.

After some testing I've found that git-daemon can very much be DoS-ed
from a single IP in it's current form. This is for two reasons:
1) The clever xcalloc + memcmp trick has a fault; the port for each
connection is different, so there will never be a match. I have a
patch[1] for this that I plan to send out soon.
2) Even with this patch the effect of the DoS-protection is kind of
limited. This is because it's a child process of the fork()'d process
again that does all the heavy lifting, and kill(pid, SIGHUP) doesn't
kill child processes. So, the connection gets to continue the action
until upload-pack (or whatever the current command is) finish. This
might be quite lengthy.

Actually, this isn't the end of the story, there's an additional issue
that happens if 1) doesn't:
In commit 02d57da (Be slightly smarter about git-daemon client
shutdown), Linus adds the following hunk:

@@ -26,6 +26,12 @@ static int upload(char *dir, int dirlen)
            access("HEAD", R_OK))
                return -1;

+       /*
+        * We'll ignore SIGTERM from now on, we have a
+        * good client.
+        */
+       signal(SIGTERM, SIG_IGN);
+
        /* git-upload-pack only ever reads stuff, so this is safe */
        execlp("git-upload-pack", "git-upload-pack", ".", NULL);
        return -1;

This is fine, because he also makes sure to first try to kill with
SIGTERM, and then fall back to SIGKILL if that failed. However,
Stephen later adds commit 3bd62c2 ("git-daemon: rewrite kindergarden,
new option --max-connections"), and here he leaves the hunk quoted
above, but removes the SIGKILL code-path. The consequence is that the
forked process doesn't die at all any more.

IMO, Stephen did kind-of right in removing the SIGKILL code-path,
since we don't kill just a random child anymore. But leaving the
SIGTERM-ignoring on looks like a bug to me.

Now, if that was fixed alone, I think we'd suffer even worse, due to
2) - since the child processes aren't killed, we'd end up allowing
more connections than the value of max_connections - upload-pack would
gladly continue in the background. Some quick testing showed that the
following patch solved the issue for me. I'm not very happy about it,
since I'm working on porting the code to Windows, and we don't have
the same process-group concept on windows... Oh well.

diff --git a/daemon.c b/daemon.c
index 918e560..bc6874c 100644
--- a/daemon.c
+++ b/daemon.c

@@ -291,12 +291,6 @@ static int run_service(char *dir, struct

daemon_service *service)
                return -1;
        }

-       /*
-        * We'll ignore SIGTERM from now on, we have a
-        * good client.
-        */
-       signal(SIGTERM, SIG_IGN);
-
        return service->fn();
 }

@@ -633,7 +627,8 @@ static void kill_some_child(void)

        for (; (next = blanket->next); blanket = next)
                if (!addrcmp(&blanket->address, &next->address)) {
-                       kill(blanket->pid, SIGTERM);
+                       kill(-blanket->pid, SIGTERM);
                        break;
                }
 }

@@ -676,7 +671,8 @@ static void handle(int incoming, struct sockaddr

*addr, int addrlen)

                add_child(pid, addr, addrlen);
                return;
-       }
+       } else
+               setpgid(0, 0);

        dup2(incoming, 0);
        dup2(incoming, 1);

-- 
Erik "kusma" Faye-Lund