Re: [PATCH] receive-pack: plug minor memory leak in unpack()

From: Jeff King <peff@peff.net>
Date: Oct 20, 2014, 09:19 UTC
Message-ID: <20141020091916.GA12913@peff.net>
In-Reply-To: <54439CDA.9070804@web.de>

On Sun, Oct 19, 2014 at 01:13:30PM +0200, René Scharfe wrote:

Show 6 quoted lines

> >We could flip it to give the managed version the short name (and calling
> >the unmanaged version "env_ptr" or something). That would require
> >munging the existing callers, but the tweak would be simple.
> 
> Perhaps, but I'm a but skeptical of big renames.  Let's start small and add
> env_array, and see how far we get with that.

Yeah, having basically implemented patches similar to yours, I think that is a good first step. Both of your patches looked good to me.

> Trickiness makes me nervous, especially in daemon.c.  And 5% CPU usage just
> for waiting sounds awful.  Using waitpid(0, ...) is not supported by the
> current implementation in compat/mingw.c, however.

I guess you could use wait() and a counter that you increment whenever you get SIGCLD, but that feels a bit hacky. I wonder how bad a real waitpid would be for mingw.

Show 6 quoted lines

> By the way, does getaddrinfo(3) show up in your profiles much?  Recently I
> looked into calling it only on demand instead of for all incoming
> connections because doing that unconditional with a user-supplied
> ("tainted") hostname just felt wrong.  The resulting patch series turned out
> to be not very pretty and I didn't see any performance improvements in my
> very limited tests, however; not sure if it's worth it.

It shows up in the child, not the parent, so it wasn't part of the profiling I did recently. I did look at it just now, and it does introduce some latency into each request (though not a lot of CPU; mainly it's the DNS request). Like you, I'm nervous about convincing git-daemon to do lookups on random hosts. By itself it's not horrible (except for tying up git-daemon with absurdly long chains of glueless references), but I worry that it could exacerbate other problems (overflows or other bugs in DNS resolvers, as part of a cache-poisoning scheme, orbeing used for DoS amplification).

I think doing it on demand would be a lot more sensible. We do not need to do a lookup at all unless the %H, %CH, or %IP interpolated path features are used. And we do not need to do hostname canonicalization unless %CH is used.

-Peff