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

From: Erik Faye-Lund <kusmabite@googlemail.com>
Date: Jan 9, 2010, 00:49 UTC
Message-ID: <40aa078e1001081649h5cb767d5t880110d923418300@mail.gmail.com>
In-Reply-To: <200912022027.23344.j6t@kdbg.org>

On Wed, Dec 2, 2009 at 8:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:

Show 20 quoted lines

> 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.

As I said, I have a patch for 1), but I don't quite know how to fix 2). Perhaps this is a good use for process groups? I'm a Windows-guy; my POSIX isn't exactly super-awesome...

I found these issues during my latest effort to port git-daemon to Windows. I managed to get this to work fine on Windows, by implementing a kill(x, SIGTERM) that terminated child-processes (because I was under the impression that this was what happened... I guess daemon.c lead me to believe that).

[1]: http://repo.or.cz/w/git/kusma.git/commit/b1d286d32f42c57b90a1db9b7b8d6775a5d1ad7b

-- 
Erik "kusma" Faye-Lund