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

[Jeff King <peff@peff.net>]

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

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

> 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