Junio C Hamano [off-list ref] writes:
But the nature of breakage caused by such an abuse is rather bad. We will
end up with loose object files, whose names do not match what are stored
and recovered when uncompressed.
This teaches the index_mem() codepath to be paranoid and hash and compress
the data after reading it in core. The contents hashed may not match the
contents of the file in an insane use case, but at least this way the
result will be internally consistent.
With a small fix to the test program earlier in the thread, this seems to
protect the repository; I didn't bother to assess the performance impact
of the patch, though.
Here is the corrected test.
-- >8 --
#!/bin/sh
set -e
# Create an empty git repo in /tmp/git-test
rm -fr /tmp/git-test
mkdir /tmp/git-test
cd /tmp/git-test
git init
# Create a file named foo and add it to the repo
touch foo
git add foo
# Thread 1: continuously modify foo:
while echo -n .; do
dd if=/dev/urandom of=foo count=1024 bs=1k conv=notrunc >/dev/null 2>&1
done &
# Thread 2: loop until the repo is corrupted
while git fsck; do
# Note the implied 'git add' in 'commit -a'
# It will do the same with explicit 'git add'
git commit -a -m'Test' || break
done
# Kill thread 1, we don't need it any more
kill $!
# Success! Well, sort of.
if git fsck
then
echo Repository is corrupted. Have a nice day.
else
echo Repository is still healthy. You are stupid.
fi
On Wed, Feb 17, 2010 at 05:20:00PM -0800, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:
With a small fix to the test program earlier in the thread, this seems to
protect the repository; I didn't bother to assess the performance impact
of the patch, though.
Here is the corrected test.
Depends on what you mean by "corrected" I suppose.
# Thread 2: loop until the repo is corrupted
while git fsck; do
# Note the implied 'git add' in 'commit -a'
# It will do the same with explicit 'git add'
git commit -a -m'Test' || break
This is at least partly wrong--it will terminate prematurely if Thread
1 gets stalled and fails to modify 'foo' during the loop (git commit
normally refuses to commit a tree with no changes). This can cause
the test for the corruption bug to return false success results.
If you add '--allow-empty' to the git commit command you will fix that
case, but there might be others.
If git commit runs out of disk space, for example, the commit should
fail, but the repository should still not be corrupt. Future commits
(for example after freeing some disk space) should eventually succeed.
Really, the original loop was correct, and this new one isn't.
else
echo Repository is still healthy. You are stupid.
If git is working, you should never reach this line, because git
fsck should not fail after executing any sequence of git porcelain
operations--and this particular sequence is nothing but 'git commit'
in a single thread.
Zygo Blaxell [off-list ref] writes:
If git commit runs out of disk space, for example, the commit should
fail, but the repository should still not be corrupt. Future commits
(for example after freeing some disk space) should eventually succeed.
That is true. It is Ok to create a corrupt object as long as running an
equivalent of fsck immediately after an object creation to catch the
breakage to prevent it from propagating further.
That is essentially what "paranoid" switch does, but it adds overhead that
is unnecessary for the use case we primarily target in git.