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

From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Date: Feb 12, 2008, 21:38 UTC
Message-ID: <20080212213858.GA29151@auto.tuwien.ac.at>
In-Reply-To: <alpine.LFD.1.00.0802121507310.2732@xanadu.home>

On Tue, Feb 12, 2008 at 03:22:17PM -0500, Nicolas Pitre wrote:

Show 8 quoted lines

> On Tue, 12 Feb 2008, Martin Koegler wrote:
> > We will need some additional memory for struct blob/tree/tag/commit
> > even for this check.
> 
> Why so?
> 
> Each received object is stored in memory when received, so why can't you 
> simply validate it in place?  No need to keep trace of them afterward.

Freeing the data is not my problem.

Many validations are in parse_XXX_buffer, which are also used by fsck. This returns a struct commit/tree/tag/blob.

I have not found any code in git to free them.

Same for pack-objects, e.g. add_objects_in_unpacked_packs allocates many struct object via lookup_unknown_object. As far as I understand the code, they are never freed, even if they are not needed later.

Show 13 quoted lines

> > > As to making sure those objects are well connected... well this is a 
> > > technically different issue entirely, and I wonder if a special mode to 
> > > fsck might not be a better solution.  For example, fsck could be made to 
> > > validate object connectivity, starting from the new ref(s), and stopping 
> > > object walking as soon as a reference to an object not included in the 
> > > newly received pack is encountered.  This could be run from some hook to 
> > > decide whether or not to update the new refs, and to delete the pack 
> > > otherwise.
> > 
> > Do you really think, that this will need less memory? fsck loads first
> > all objects and then verifies their connections.
> 
> Not all objects otherwise I wouldn't even be able to run it.

It only loads all objects, it checks (eg. no objects from the pack file by default). After loading a object, it frees the content of object, but keeps the parsed information in memory for the connectivity check.

Show 5 quoted lines

> My point is that you can have fsck load only objects contained in the 
> received pack (you can use the pack index to load them) and assume 
> connectivity is good whenever an object in the pack reference an 
> existing object outside of the pack.  At least this doesn't need to 
> happen in parallel with pack indexing.

So you propose the following:
1) run index-pack (write pack file + index in repository)
2) fsck pack file
3) update refs

This looks to be very prone to race conditions, if multiple pushes run concurrently.

To be on the safe side, the checks must be finished, before a pack/object becomes part of the repository. We must check the reachability before index-pack writes the index.

In my opinion, separating the code for the reachability check from index-pack would complicate things. It would not safe memory, as the memory would be used by two processes instead of one.

You don't want to increase the resource usage of index-pack. If --strict is not passed, I implemented this.

For --strict, it is not avoidable, that index-pack will use more memory and cpu time.

mfg Martin Kögler