upload-pack is unsafe in corrupt repository

upload-pack is unsafe in corrupt repository

[Junio C Hamano]

I was helping Andrew fetching from maintainer trees.  One of the
trees were corrupt (i.e. git-fsck-objects unclean) and there was
a rather grave consequences we found out the hard way (and it is
5am but Tuesday is not my git day X-<).

upload-pack forks a pipeline, whose upstream execs rev-list and
downstream execs pack-objects.  rev-list correctly notices that
it cannot reach some objects and dies, but pack-objects just
says "Ah, the list of object input just ended -- let's send them
all".  This results in git-fetch-pack to be fed an incomplete
pack.  However, git-unpack-objects is happy (the pack is not
corrupt, just that it does not contain what the protocol
exchange expected to send) and git-fetch-pack ends up going all
the way through updating the ref.

Eek.

At this moment, I am just reporting this because I am too tired
to code it myself right now, but I think the fix is to make
upload-pack to fork once more, waitpid() for rev-list and
inject/append garbage in the stream pack-objects produces, in
order to cause unpack-objects (or index-pack) that runs on the
the fetching end to notice, when rev-list or pack-objects barf.

Of course a nicer fix is to add the sideband we have been
discussing about, but that relies on protocol extension and
would not solve the problem for older clients.

Side note: in my head, the sideband would work by the "forked
once more" upload-pack process to listen to both stdout and
stderr of pack-objects, and feed them packetized (instead of the
current "go-straight-stream") by perhaps prefixing them with one
byte (is this a payload, or is this a progress-bar/keepalive type
packet).  Of course, this would change the way the payload is
transferred, so the initial handshake needs to negotiate if the
sideband should be used just like we do other new features like
thin pack transfers.