Re: 'git add' corrupts repository if the working directory is modified as it runs

From: Thomas Rast <trast@student.ethz.ch>
To: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Cc: Jonathan Nieder <jrnieder@gmail.com>,
	Zygo Blaxell <zblaxell@gibbs.hungrycats.org>,
	<git@vger.kernel.org>
Subject: Re: 'git add' corrupts repository if the working directory is modified as it runs
Date: Sat, 13 Feb 2010 15:39:53 +0100	[thread overview]
Message-ID: <201002131539.54142.trast@student.ethz.ch> (raw)
In-Reply-To: <20100213133951.GA14352@Knoppix>

On Saturday 13 February 2010 14:39:52 Ilari Liusvaara wrote:
> On Sat, Feb 13, 2010 at 06:12:38AM -0600, Jonathan Nieder wrote:
> > 
> > With the current code, write_sha1_file() will hash the file, notice
> > that object is already in .git/objects, and return.  With a
> > read-hash-copy loop, git would have to store a (compressed or
> > uncompressed) copy of the file somewhere in the meantime.
> 
> It could be done by first reading the file and computing hash,
> if the hash matches existing object, return that hash. Otherwise
> read the file for object write, hashing it again and use that value
> for object ID.

That is still racy.  The real problem is that the file is mmap()ed,
and git then first computes the SHA1 of that buffer, next it
compresses it.[*]

Due to the last sentence in the following snippet from mmap(2):

  MAP_PRIVATE
           Create a private copy-on-write mapping.  Updates to the map-
           ping are not visible to other  processes  mapping  the  same
           file,  and  are  not carried through to the underlying file.
           It is unspecified whether changes made to the file after the
           mmap() call are visible in the mapped region.

This is racy despite the use of MAP_PRIVATE: the mapped contents can
change at any time.

AFAICS there are only two possible solutions:

* Copy the file (possibly block-by-block) as we go, to make sure that
  the data we SHA1 is the same we compress.

* Unpack and re-hash the compressed data to verify that the SHA1 is
  correct.  In case of failure either retry (but you could have to do
  this infinitely often if the user just hates you!) or abort.

(Of course, in neither case does the user have any sort of guarantee
about what data ended up in the repository, but he never had that, we
only try to ensure repo consistency.)

[*] The "do we have this" check actually happens before the
compression, and that arm is thus race-free.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch