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

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Sat, Jan 9, 2010 at 1:49 AM, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Wed, Dec 2, 2009 at 8:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:
>>> On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>> > "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