Re: [RFC Patch] Preventing corrupt objects from entering the repository

Nicolas Pitre <nico@cam.org> wrote:

We have *waaaay* overthought this. The solution is a lot simpler than Martin's patches make it out to be, and we can do it without changing our current memory footprint.

When we get the raw data for an object so we can compute its SHA-1 and/or write its loose object to disk we should first verify its content is sane, then do the SHA-1/store loose.

Blobs - always assume sane. Remember that these make up the largest percentage in terms of raw bytes of any project's packfiles and we don't need to do any additional processing to them. Yay us.

Tags - stack allocate a temporary struct tag and pass it's address into parse_tag_buffer(). The only thing it tries to allocate is the item->tagged. Throw an extra argument into parse_tag_buffer() to bypass this lookup block. Now there's no extra memory used for tags, just a bit more CPU. Tags aren't frequent.

Commits - do like tags, but use parse_commit_buffer(). Avoid looking up the tree object and the parents, and allocating the parent references via a new argument. Also check !item->date like fsck does. Other than that I think the validation code in fsck is redundant with what parse_commit_buffer() is doing already.

Trees - just run init_tree_desc() and tree_entry() loop like in track_tree_refs() (and a ton of other places) to iterate over the buffer. All we care about is sane entry names (no '/'), sane modes, and correct sorting.

In short, we ignore reachability, but get fsck, and we can completely avoid additional malloc activity as well as any sort of heap increase in index-pack and unpack-objects. Its not hard.

Its a small amount of refactoring for the parse functions we already have, and maybe expose a function or two from builtin-fsck (in particular a bulk of fsck_tree should be reused).

Yea, its a tad more CPU time, but it shouldn't be that costly here. The huge cost in fsck is redoing the SHA-1 checksums, and inflating the deltas. We've already got the delta inflated, and we're about to compute the SHA-1 checksum for the first time. So those major costs of fsck drop to 0.

Nah, just do what quickfetch does in builtin-fetch.c, but run it in receive-pack, between unpack() and execute_commands():

	rev-list --quiet --objects $new... --not --all

If it aborts, reachability testing failed and the push is rejected without updating any refs. Yes your repository now has objects that are missing things, but none of those are considered to be reachable, so this isn't a big deal. They will get cleaned up on the next `gc --prune`, whenever that is.

In this configuration (--quiet) rev-list tries to be pretty low on its memory usage, it doesn't save buffers, etc. Further since everything that is already considered reachable is not interesting, we are only doing a walk over the objects that we just received, not our entire ODB. Its also after index-pack exited, so we just freed up a good chunk of memory.

Rememeber we are talking about receive-pack here. The cost on the to perform the rev-list is lower than the cost will be to pack these objects for distribution back to just one client. Since this is a server of some sorts (otherwise why did you push here?), odds are its going to be doing a lot of packing requests for clients to receive these newly uploaded objects by the native git protocol. This new rev-list is nothing compared to that already existing load. And if your OS is any good the just created .idx and .pack is still in OS buffer cache, so there shouldn't be any additional disk IO.

Yes, we could make this optional in receive-pack, but really I don't see a reason to. Just run it. The client shouldn't be giving us unreachable crap.