Re: git bugs

From: Ben Lynn <benlynn@gmail.com>
Date: Jun 10, 2008, 23:09 UTC
Message-ID: <832adb090806101609q17a21948nb5814c3b22bd832d@mail.gmail.com>
In-Reply-To: <alpine.LFD.1.10.0806101249580.3101@woody.linux-foundation.org>

Show 7 quoted lines

>> Nice! I believe I can prove there are no races now.
>
> It's worth pointing out that even in the absense of races, you can
> obviously screw things up if you really work at it, and *want* to. We
> cannot guarantee that we see all file changes from the stat() information,
> and we don't even save the whole stat info (ie we only save the low 32
> bits).

I agree completely. My proof only holds in an abstract setting. It assumes things such as a strictly nondecreasing system clock is and that ctime and mtime work in an ideal sense.

> And different systems have different approaches to what happens when a
> file gets modified through a writable mmap(). Exactly what is the mtime
> going to be?

Good point. I confess I've only learned about mmap very recently, from browsing the git code. All this time, I've been using streams, file descriptors, etc. mmap is so much better!

What's Linux do in this case? For indexing purposes, so long as the mtime is updated after the last write before git gets to it, things should be fine.

> So I think git does a really good job at matching the stat() information,
> and the suggested patch makes it even stricter, but I think we should not
> even try to make it handle "malicious" changes. I bet you can work at
> making it miss some update if you really *really* try.

Definitely. e.g. rig the mtime by 2^32 seconds, or add 2^32 bytes to a file within a second.

Show 6 quoted lines

> And I think there is one known race: the index mtime itself is not
> race-free. Remember: it may take more than a second to write the index
> file! So I can imagine that if you can set it up so that you change the
> file as the index is written out, and the index write is delayed
> sufficiently, the racy timestamp logic can fail just because the timestamp
> on the index file ends up being later.

I had thought about this. I hacked some code up where the index looks at the current system time when updating a cache entry to determine if the hash is racy. Is doing one time(NULL) call per file reasonable? I'm guessing it must be cheaper that a stat call.

> This is more easily shown by doing a 'touch' on the index file afterwards,
> of course.

Agreed. Another assumption of my proof is that the index is trustworthy. If you tamper with it, all bets are off. You can't stop determined users from hurting themselves.

> And yes, we should have written the timestamp to the file itself, instead
> of reading it from the filesystem.

Interesting. I had hacked a version of the index that did this (before changing it to use a different solution).

In general, is the format of the index file set in stone? Is that why it's better to use the size zero trick for the race condition rather than introduce a new flag for example? Or are these wrinkles too small to justify a potentially painful upgrade?

-Ben