Junio C Hamano <junkio@xxxxxxx> writes: > ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes: > >> Can we fix the check in upload-pack.c something like my >> patch below does? Are there any security implications for >> doing that? > >> Could we just make the final check before dying if (!o) ? > > The primary implication is about correctness, so I am reluctant > to break it without a careful alternative check in place. > > The issue is that having a single object in the repository does > not guarantee that you have everything reachable from it, and we > need that guarantee. Reachability from the refs is what > guarantees that. I don't see why having something reachable from a ref guarantees that everything is reachable. Given the recent patch that added a check to make certain a ref actually existed I believe there is some evidence that trees may become corrupted, and have the problems you describe. > We are careful to update the ref at the very end of the transfer > (fetch/clone or push); so if an object is reachable from a ref, > then all the objects reachable from that object are available in > the repository. In the normal case I agree. > Imagine http commit walker started fetching tip of upstream into > your repository and you interrupted the transfer. Objects near > the tip of the upstream history are available after such an > interrupted transfer. But a bit older history (but still later > than what we had before we started the transfer) are not. > > We do not update the ref with the downloaded tip object, so that > we would not break the guarantee. This guarantee is needed for > feeding clients from the repository later. If you tell your > clients, after such an interrupted transfer, that you are > willing to serve the objects near the (new) tip, the clients may > rightfully request objects that are reachable from these > objects, some of them you do _not_ have! I clearly would not advertise it. My problem is that I have evidence that someone pulled a given sha1 at some point from some branch on a given repository. But I don't have that branch. Actually trees mirrored with rsync have similar problems all of the time when the catch a tree in the middle of an update. > So this "on demand SHA1" stuff needs to be solved by checking if > the given object is reachable from our refs in upload-pack, > instead of the current check to see if the given object is > pointed by our refs. When upload-pack can prove that the object > is reachable from one of the refs, it is OK to use it; otherwise > you should not. I have a problem with that approach. Suppose the branch I have evidence something came from is like your pu branch. If I want a copy of your pu branch at some point in the past, but you have rebased it since that sha1 was published then there will clearly not be a path from any current head to that branch. But if I still have a copy of the sha1 I should actually be able to recover the old copy of the pu branch from your tree. > Now, proving that a given SHA1 is the name of an object that > exists in the repository is cheap (has_sha1_file()), but proving > that the object is reachable from some of our refs can become > quite expensive. That gives this issue a security implication > as well -- you can easily DoS the git-daemon that way, for > example. Exactly, which is why I aimed for the cheap test. There is a reasonable argument that can be made that the branches represent the policy that you are willing to serve. If you have a tree and share a common object store with a much lager tree, like David Woodhouse has set up, I can see such a policy being desirable. That is an argument I have a much harder time shooting down. At the same time if it is just a policy question the policy it should be modifiable with an appropriate configuration directive, or command line option. Eric - : send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html