[JGIT PATCH 1/2] index-pack: Avoid disk corruption yielding a valid pack footer checksum

From: Shawn O. Pearce <spearce@spearce.org>
Date: Aug 27, 2008, 21:48 UTC
Message-ID: <1219873724-13348-1-git-send-email-spearce@spearce.org>

When we are processing a thin pack and making it non-thin we need to update the header with a new object count. That causes us to recompute the footer checksum for the entire pack, and the only way to do that is to re-read the data from disk.

If there was filesystem corruption in the process (e.g. a bad disk sector, or a kernel bug) we don't want to produce a valid pack at the end. Instead we need to fail-fast with the error so the user is aware of the corruption.

We now keep track of where the end of the original data is and run two SHA-1 computations during the header-footer fixup. If the original data region doesn't match the original footer we got over the network we know there was corruption and we just cannot trust this pack file.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

  This was inspired by the data corruption thread started
  by J. Bruce Fields.  At some point in the thread Linus
  pointed out the C Git index-pack isn't as safe as it can
  be, and offered a strategy to fix it.

  We weren't as safe as we could be either in jgit, even
  though we do verify CRC codes when we re-read a delta
  base from disk.  Not all objects are used as a base and
  thus we could miss some forms of corruption.

 .../src/org/spearce/jgit/transport/IndexPack.java  |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
index 29d99db..db9268e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
@@ -156,6 +156,9 @@ public static IndexPack create(final Repository db, final InputStream is)
 
 	private byte[] packcsum;
 
+	/** If {@link #fixThin} this is the last byte of the original checksum. */
+	private long originalEOF;
+
 	/**
 	 * Create a new pack indexer utility.
 	 * 
@@ -398,8 +401,9 @@ private void resolveChildDeltas(final long pos, int type, byte[] data,
 	private void fixThinPack(final ProgressMonitor progress) throws IOException {
 		growEntries();
 
+		originalEOF = packOut.length() - 20;
 		final Deflater def = new Deflater(Deflater.DEFAULT_COMPRESSION, false);
-		long end = packOut.length() - 20;
+		long end = originalEOF;
 		for (final ObjectId baseId : new ArrayList<ObjectId>(baseById.keySet())) {
 			final ObjectLoader ldr = repo.openObject(baseId);
 			if (ldr == null)
@@ -453,9 +457,13 @@ private void writeWhole(final Deflater def, final int typeCode,
 	}
 
 	private void fixHeaderFooter() throws IOException {
+		final MessageDigest origDigest = Constants.newMessageDigest();
+		long origRemaining = originalEOF - 12;
+
 		packOut.seek(0);
 		if (packOut.read(buf, 0, 12) != 12)
 			throw new IOException("Cannot re-read pack header to fix count");
+		origDigest.update(buf, 0, 12);
 		NB.encodeInt32(buf, 8, entryCount);
 		packOut.seek(0);
 		packOut.write(buf, 0, 12);
@@ -466,9 +474,16 @@ private void fixHeaderFooter() throws IOException {
 			final int n = packOut.read(buf);
 			if (n < 0)
 				break;
+			if (origRemaining > 0) {
+				origDigest.update(buf, 0, (int) Math.min(n, origRemaining));
+				origRemaining -= n;
+			}
 			packDigest.update(buf, 0, n);
 		}
 
+		if (!Arrays.equals(origDigest.digest(), packcsum))
+			throw new IOException("Pack corrupted while writing to filesystem");
+
 		packcsum = packDigest.digest();
 		packOut.write(packcsum);
 	}

-- 
1.6.0.174.gd789c