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

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

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.

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