{"thread":{"id":"15226","subject":"[JGIT PATCH 2/2] index-pack: Use fsync to ensure received pack data is on disk","startedAt":"2008-08-27T21:48:43Z","lastAt":"2008-08-28T00:26:07Z","messageCount":5,"participants":["Shawn O. Pearce","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"88792","messageId":"1219873724-13348-1-git-send-email-spearce@spearce.org","threadId":"15226","inReplyTo":null,"subject":"[JGIT PATCH 1/2] index-pack: Avoid disk corruption yielding a valid pack footer checksum","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-27T21:48:43Z","receivedAt":"2008-08-27T21:48:43Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"When we are processing a thin pack and making it non-thin we need\nto update the header with a new object count. That causes us to\nrecompute the footer checksum for the entire pack, and the only\nway to do that is to re-read the data from disk.\n\nIf there was filesystem corruption in the process (e.g. a bad\ndisk sector, or a kernel bug) we don't want to produce a valid\npack at the end. Instead we need to fail-fast with the error\nso the user is aware of the corruption.\n\nWe now keep track of where the end of the original data is and\nrun two SHA-1 computations during the header-footer fixup. If\nthe original data region doesn't match the original footer we\ngot over the network we know there was corruption and we just\ncannot trust this pack file.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n This was inspired by the data corruption thread started\n by J. Bruce Fields. At some point in the thread Linus\n pointed out the C Git index-pack isn't as safe as it can\n be, and offered a strategy to fix it.\n\n We weren't as safe as we could be either in jgit, even\n though we do verify CRC codes when we re-read a delta\n base from disk. Not all objects are used as a base and\n thus we could miss some forms of corruption.\n\n .../src/org/spearce/jgit/transport/IndexPack.java | 17 ++++++++++++++++-\n 1 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\nindex 29d99db..db9268e 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n@@ -156,6 +156,9 @@ public static IndexPack create(final Repository db, final InputStream is)\n \n \tprivate byte[] packcsum;\n \n+\t/** If {@link #fixThin} this is the last byte of the original checksum. */\n+\tprivate long originalEOF;\n+\n \t/**\n \t * Create a new pack indexer utility.\n \t * \n@@ -398,8 +401,9 @@ private void resolveChildDeltas(final long pos, int type, byte[] data,\n \tprivate void fixThinPack(final ProgressMonitor progress) throws IOException {\n \t\tgrowEntries();\n \n+\t\toriginalEOF = packOut.length() - 20;\n \t\tfinal Deflater def = new Deflater(Deflater.DEFAULT_COMPRESSION, false);\n-\t\tlong end = packOut.length() - 20;\n+\t\tlong end = originalEOF;\n \t\tfor (final ObjectId baseId : new ArrayList(baseById.keySet())) {\n \t\t\tfinal ObjectLoader ldr = repo.openObject(baseId);\n \t\t\tif (ldr == null)\n@@ -453,9 +457,13 @@ private void writeWhole(final Deflater def, final int typeCode,\n \t}\n \n \tprivate void fixHeaderFooter() throws IOException {\n+\t\tfinal MessageDigest origDigest = Constants.newMessageDigest();\n+\t\tlong origRemaining = originalEOF - 12;\n+\n \t\tpackOut.seek(0);\n \t\tif (packOut.read(buf, 0, 12) != 12)\n \t\t\tthrow new IOException(\"Cannot re-read pack header to fix count\");\n+\t\torigDigest.update(buf, 0, 12);\n \t\tNB.encodeInt32(buf, 8, entryCount);\n \t\tpackOut.seek(0);\n \t\tpackOut.write(buf, 0, 12);\n@@ -466,9 +474,16 @@ private void fixHeaderFooter() throws IOException {\n \t\t\tfinal int n = packOut.read(buf);\n \t\t\tif (n < 0)\n \t\t\t\tbreak;\n+\t\t\tif (origRemaining > 0) {\n+\t\t\t\torigDigest.update(buf, 0, (int) Math.min(n, origRemaining));\n+\t\t\t\torigRemaining -= n;\n+\t\t\t}\n \t\t\tpackDigest.update(buf, 0, n);\n \t\t}\n \n+\t\tif (!Arrays.equals(origDigest.digest(), packcsum))\n+\t\t\tthrow new IOException(\"Pack corrupted while writing to filesystem\");\n+\n \t\tpackcsum = packDigest.digest();\n \t\tpackOut.write(packcsum);\n \t}\n-- \n1.6.0.174.gd789c\n"},{"id":"88791","messageId":"1219873724-13348-2-git-send-email-spearce@spearce.org","threadId":"15226","inReplyTo":"1219873724-13348-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH 2/2] index-pack: Use fsync to ensure received pack data is on disk","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-27T21:48:44Z","receivedAt":"2008-08-27T21:48:44Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"C Git uses fsync() to ensure that a pack file is stable on disk before\nit starts to delete any other copies of the data, or before it tries\nto update refs to refer to data stored in the pack. This makes it a\nlot less likely that a repository will see corruption due to refs\npointing at unavailable objects.\n\nWe now force our pack and its associated .idx file to disk before\nwe close the file descriptors, but after we have finished reading\nand writing all of the data. This way we can be fairly certain\na future user of the data (like a RefUpdate) always has objects.\n\nSigned-off-by: Shawn O. Pearce \n---\n .../src/org/spearce/jgit/transport/IndexPack.java | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\nindex db9268e..7b1f7ee 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n@@ -259,6 +259,8 @@ public void index(final ProgressMonitor progress) throws IOException {\n \t\t\t\t\t\tfixThinPack(progress);\n \t\t\t\t\t}\n \t\t\t\t}\n+\t\t\t\tif (packOut != null)\n+\t\t\t\t\tpackOut.getChannel().force(true);\n \n \t\t\t\tpackDigest = null;\n \t\t\t\tbaseById = null;\n@@ -510,6 +512,7 @@ private void writeIdx() throws IOException {\n \t\t\telse\n \t\t\t\tiw = PackIndexWriter.createVersion(os, outputVersion);\n \t\t\tiw.write(list, packcsum);\n+\t\t\tos.getChannel().force(true);\n \t\t} finally {\n \t\t\tos.close();\n \t\t}\n-- \n1.6.0.174.gd789c\n"},{"id":"88829","messageId":"alpine.LFD.1.10.0808272007541.1624@xanadu.home","threadId":"15226","inReplyTo":"1219873724-13348-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 1/2] index-pack: Avoid disk corruption yielding a valid pack footer checksum","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-28T00:09:49Z","receivedAt":"2008-08-28T00:09:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 27 Aug 2008, Shawn O. Pearce wrote:\n\n> When we are processing a thin pack and making it non-thin we need\n> to update the header with a new object count. That causes us to\n> recompute the footer checksum for the entire pack, and the only\n> way to do that is to re-read the data from disk.\n> \n> If there was filesystem corruption in the process (e.g. a bad\n> disk sector, or a kernel bug) we don't want to produce a valid\n> pack at the end. Instead we need to fail-fast with the error\n> so the user is aware of the corruption.\n> \n> We now keep track of where the end of the original data is and\n> run two SHA-1 computations during the header-footer fixup. If\n> the original data region doesn't match the original footer we\n> got over the network we know there was corruption and we just\n> cannot trust this pack file.\n> \n> Signed-off-by: Shawn O. Pearce \n> ---\n> \n> This was inspired by the data corruption thread started\n> by J. Bruce Fields. At some point in the thread Linus\n> pointed out the C Git index-pack isn't as safe as it can\n> be, and offered a strategy to fix it.\n\nI thought it was Nicolas Pitre who offered that strategy?\nMaybe I was mistaken. ;-)\n\n\nNicolas\n"},{"id":"88830","messageId":"20080828001401.GT26523@spearce.org","threadId":"15226","inReplyTo":"alpine.LFD.1.10.0808272007541.1624@xanadu.home","subject":"Re: [JGIT PATCH 1/2] index-pack: Avoid disk corruption yielding a valid pack footer checksum","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-28T00:14:01Z","receivedAt":"2008-08-28T00:14:01Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Wed, 27 Aug 2008, Shawn O. Pearce wrote:\n> > \n> > This was inspired by the data corruption thread started\n> > by J. Bruce Fields. At some point in the thread Linus\n> > pointed out the C Git index-pack isn't as safe as it can\n> > be, and offered a strategy to fix it.\n> \n> I thought it was Nicolas Pitre who offered that strategy?\n> Maybe I was mistaken. ;-)\n\nSorry about the bad attribution. Going back through the\narchives does say it was you that had the brilliant idea.\n\nHeh. Well at least my fuzzy memory of recent history is\nonly on the mailing list archives and not in a commit.\n\n;-)\n\n-- \nShawn.\n"},{"id":"88833","messageId":"alpine.LFD.1.10.0808272024160.1624@xanadu.home","threadId":"15226","inReplyTo":"20080828001401.GT26523@spearce.org","subject":"Re: [JGIT PATCH 1/2] index-pack: Avoid disk corruption yielding a valid pack footer checksum","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-28T00:26:07Z","receivedAt":"2008-08-28T00:26:07Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 27 Aug 2008, Shawn O. Pearce wrote:\n\n> Nicolas Pitre wrote:\n> > On Wed, 27 Aug 2008, Shawn O. Pearce wrote:\n> > > \n> > > This was inspired by the data corruption thread started\n> > > by J. Bruce Fields. At some point in the thread Linus\n> > > pointed out the C Git index-pack isn't as safe as it can\n> > > be, and offered a strategy to fix it.\n> > \n> > I thought it was Nicolas Pitre who offered that strategy?\n> > Maybe I was mistaken. ;-)\n> \n> Sorry about the bad attribution. Going back through the\n> archives does say it was you that had the brilliant idea.\n\nThanks. Now my ego is fully restored. ;-)\n\n\nNicolas\n"}]}