From 6e5c71b358e9b9b883f24f073e869ff6affe5bf4 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 24 Apr 2015 11:16:33 -0700 Subject: [PATCH] Remove validate support when reusing cached pack Cached packs are only used when writing over the network or to a bundle file and reuse validation is always disabled in these two contexts. The client/consumer of the stream will be SHA-1 checksumming every object. Reuse validation is most critical during local GC to avoid silently ignoring corruption by stopping as soon as a problem is found and leaving everything alone for the end-user to debug and salvage. Cached packs are not supported during local GC as the bitmap rebuild logic does not support including a cached pack in the result. Strip out the validation and force PackWriter to always disable the cached pack feature if reuseValidation is enabled. Change-Id: If0d7baf2ae1bf1f7e71bf773151302c9f7887039 --- .../jgit/internal/storage/dfs/DfsBlock.java | 8 +-- .../internal/storage/dfs/DfsCachedPack.java | 5 +- .../internal/storage/dfs/DfsPackFile.java | 66 ++++--------------- .../jgit/internal/storage/dfs/DfsReader.java | 6 +- .../storage/file/ByteArrayWindow.java | 5 +- .../storage/file/ByteBufferWindow.java | 5 +- .../internal/storage/file/ByteWindow.java | 5 +- .../storage/file/LocalCachedPack.java | 4 +- .../jgit/internal/storage/file/PackFile.java | 6 +- .../internal/storage/file/WindowCursor.java | 47 ++----------- .../storage/pack/ObjectReuseAsIs.java | 9 +-- .../internal/storage/pack/PackWriter.java | 4 +- 12 files changed, 38 insertions(+), 132 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java index c1853327f..79265363e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java @@ -46,7 +46,6 @@ package org.eclipse.jgit.internal.storage.dfs; import java.io.IOException; -import java.security.MessageDigest; import java.util.zip.CRC32; import java.util.zip.DataFormatException; import java.util.zip.Inflater; @@ -101,12 +100,9 @@ void crc32(CRC32 out, long pos, int cnt) { out.update(block, ptr, cnt); } - void write(PackOutputStream out, long pos, int cnt, MessageDigest digest) + void write(PackOutputStream out, long pos, int cnt) throws IOException { - int ptr = (int) (pos - start); - out.write(block, ptr, cnt); - if (digest != null) - digest.update(block, ptr, cnt); + out.write(block, (int) (pos - start), cnt); } void check(Inflater inf, byte[] tmp, long pos, int cnt) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java index 3da5184e0..a5308f617 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java @@ -78,8 +78,7 @@ public boolean hasObject(ObjectToPack obj, StoredObjectRepresentation rep) { return ((DfsObjectRepresentation) rep).pack == pack; } - void copyAsIs(PackOutputStream out, boolean validate, DfsReader ctx) - throws IOException { - pack.copyPackAsIs(out, validate, ctx); + void copyAsIs(PackOutputStream out, DfsReader ctx) throws IOException { + pack.copyPackAsIs(out, ctx); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java index b51ee2f4d..e03488b3c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java @@ -56,9 +56,7 @@ import java.io.InputStream; import java.nio.ByteBuffer; import java.nio.channels.Channels; -import java.security.MessageDigest; import java.text.MessageFormat; -import java.util.Arrays; import java.util.Set; import java.util.zip.CRC32; import java.util.zip.DataFormatException; @@ -466,55 +464,35 @@ private byte[] decompress(long position, int sz, DfsReader ctx) return dstbuf; } - void copyPackAsIs(PackOutputStream out, boolean validate, DfsReader ctx) + void copyPackAsIs(PackOutputStream out, DfsReader ctx) throws IOException { - MessageDigest md = initCopyPack(out, validate, ctx); - long p; - if (cache.shouldCopyThroughCache(length)) - p = copyPackThroughCache(out, ctx, md); - else - p = copyPackBypassCache(out, ctx, md); - verifyPackChecksum(p, md, ctx); - } - - private MessageDigest initCopyPack(PackOutputStream out, boolean validate, - DfsReader ctx) throws IOException { // If the length hasn't been determined yet, pin to set it. - if (length == -1) + if (length == -1) { ctx.pin(this, 0); - if (!validate) { ctx.unpin(); - return null; } - - int hdrlen = 12; - byte[] buf = out.getCopyBuffer(); - int n = ctx.copy(this, 0, buf, 0, hdrlen); - ctx.unpin(); - if (n != hdrlen) - throw packfileIsTruncated(); - MessageDigest md = Constants.newMessageDigest(); - md.update(buf, 0, hdrlen); - return md; + if (cache.shouldCopyThroughCache(length)) + copyPackThroughCache(out, ctx); + else + copyPackBypassCache(out, ctx); } - private long copyPackThroughCache(PackOutputStream out, DfsReader ctx, - MessageDigest md) throws IOException { + private void copyPackThroughCache(PackOutputStream out, DfsReader ctx) + throws IOException { long position = 12; long remaining = length - (12 + 20); while (0 < remaining) { DfsBlock b = cache.getOrLoad(this, position, ctx); int ptr = (int) (position - b.start); int n = (int) Math.min(b.size() - ptr, remaining); - b.write(out, position, n, md); + b.write(out, position, n); position += n; remaining -= n; } - return position; } - private long copyPackBypassCache(PackOutputStream out, DfsReader ctx, - MessageDigest md) throws IOException { + private long copyPackBypassCache(PackOutputStream out, DfsReader ctx) + throws IOException { try (ReadableChannel rc = ctx.db.openFile(packDesc, PACK)) { ByteBuffer buf = newCopyBuffer(out, rc); if (ctx.getOptions().getStreamPackBufferSize() > 0) @@ -526,7 +504,7 @@ private long copyPackBypassCache(PackOutputStream out, DfsReader ctx, if (b != null) { int ptr = (int) (position - b.start); int n = (int) Math.min(b.size() - ptr, remaining); - b.write(out, position, n, md); + b.write(out, position, n); position += n; remaining -= n; rc.position(position); @@ -540,8 +518,6 @@ private long copyPackBypassCache(PackOutputStream out, DfsReader ctx, else if (n > remaining) n = (int) remaining; out.write(buf.array(), 0, n); - if (md != null) - md.update(buf.array(), 0, n); position += n; remaining -= n; } @@ -557,22 +533,6 @@ private ByteBuffer newCopyBuffer(PackOutputStream out, ReadableChannel rc) { return ByteBuffer.wrap(copyBuf, 0, bs); } - private void verifyPackChecksum(long position, MessageDigest md, - DfsReader ctx) throws IOException { - if (md != null) { - byte[] buf = new byte[20]; - byte[] actHash = md.digest(); - if (ctx.copy(this, position, buf, 0, 20) != 20) - throw packfileIsTruncated(); - if (!Arrays.equals(actHash, buf)) { - invalid = true; - throw new IOException(MessageFormat.format( - JGitText.get().packfileCorruptionDetected, - getPackName())); - } - } - } - void copyAsIs(PackOutputStream out, DfsObjectToPack src, boolean validate, DfsReader ctx) throws IOException, StoredObjectRepresentationNotAvailableException { @@ -719,7 +679,7 @@ void copyAsIs(PackOutputStream out, DfsObjectToPack src, // and we have it pinned. Write this out without copying. // out.writeHeader(src, inflatedLength); - quickCopy.write(out, dataOffset, (int) dataLength, null); + quickCopy.write(out, dataOffset, (int) dataLength); } else if (dataLength <= buf.length) { // Tiny optimization: Lots of objects are very small deltas or diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java index dc8552e49..f5f3375fa 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java @@ -492,9 +492,9 @@ public void writeObjects(PackOutputStream out, List list) out.writeObject(otp); } - public void copyPackAsIs(PackOutputStream out, CachedPack pack, - boolean validate) throws IOException { - ((DfsCachedPack) pack).copyAsIs(out, validate, this); + public void copyPackAsIs(PackOutputStream out, CachedPack pack) + throws IOException { + ((DfsCachedPack) pack).copyAsIs(out, this); } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java index 863c553b3..dc720bc62 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java @@ -46,7 +46,6 @@ package org.eclipse.jgit.internal.storage.file; import java.io.IOException; -import java.security.MessageDigest; import java.util.zip.CRC32; import java.util.zip.DataFormatException; import java.util.zip.Inflater; @@ -84,12 +83,10 @@ void crc32(CRC32 out, long pos, int cnt) { } @Override - void write(PackOutputStream out, long pos, int cnt, MessageDigest digest) + void write(PackOutputStream out, long pos, int cnt) throws IOException { int ptr = (int) (pos - start); out.write(array, ptr, cnt); - if (digest != null) - digest.update(array, ptr, cnt); } void check(Inflater inf, byte[] tmp, long pos, int cnt) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java index 31925d28e..05ddd69b3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java @@ -47,7 +47,6 @@ import java.io.IOException; import java.nio.ByteBuffer; -import java.security.MessageDigest; import java.util.zip.DataFormatException; import java.util.zip.Inflater; @@ -76,7 +75,7 @@ protected int copy(final int p, final byte[] b, final int o, int n) { } @Override - void write(PackOutputStream out, long pos, int cnt, MessageDigest digest) + void write(PackOutputStream out, long pos, int cnt) throws IOException { final ByteBuffer s = buffer.slice(); s.position((int) (pos - start)); @@ -86,8 +85,6 @@ void write(PackOutputStream out, long pos, int cnt, MessageDigest digest) int n = Math.min(cnt, buf.length); s.get(buf, 0, n); out.write(buf, 0, n); - if (digest != null) - digest.update(buf, 0, n); cnt -= n; } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java index ab5eb7c90..e774a1464 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java @@ -45,7 +45,6 @@ package org.eclipse.jgit.internal.storage.file; import java.io.IOException; -import java.security.MessageDigest; import java.util.zip.DataFormatException; import java.util.zip.Inflater; @@ -121,8 +120,8 @@ final int copy(long pos, byte[] dstbuf, int dstoff, int cnt) { */ protected abstract int copy(int pos, byte[] dstbuf, int dstoff, int cnt); - abstract void write(PackOutputStream out, long pos, int cnt, - MessageDigest md) throws IOException; + abstract void write(PackOutputStream out, long pos, int cnt) + throws IOException; final int setInput(long pos, Inflater inf) throws DataFormatException { return setInput((int) (pos - start), inf); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java index b70ebcf9e..fd9dcdafa 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java @@ -79,10 +79,10 @@ public long getObjectCount() throws IOException { return cnt; } - void copyAsIs(PackOutputStream out, boolean validate, WindowCursor wc) + void copyAsIs(PackOutputStream out, WindowCursor wc) throws IOException { for (PackFile pack : getPacks()) - pack.copyPackAsIs(out, validate, wc); + pack.copyPackAsIs(out, wc); } @Override diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java index ad3322e0d..75c361e10 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java @@ -344,11 +344,11 @@ private final byte[] decompress(final long position, final int sz, return dstbuf; } - void copyPackAsIs(PackOutputStream out, boolean validate, WindowCursor curs) + void copyPackAsIs(PackOutputStream out, WindowCursor curs) throws IOException { // Pin the first window, this ensures the length is accurate. curs.pin(this, 0); - curs.copyPackAsIs(this, length, validate, out); + curs.copyPackAsIs(this, length, out); } final void copyAsIs(PackOutputStream out, LocalObjectToPack src, @@ -502,7 +502,7 @@ private void copyAsIs2(PackOutputStream out, LocalObjectToPack src, // and we have it pinned. Write this out without copying. // out.writeHeader(src, inflatedLength); - quickCopy.write(out, dataOffset, (int) dataLength, null); + quickCopy.write(out, dataOffset, (int) dataLength); } else if (dataLength <= buf.length) { // Tiny optimization: Lots of objects are very small deltas or diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java index 85c3c7425..21d6cd2ca 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java @@ -45,9 +45,6 @@ package org.eclipse.jgit.internal.storage.file; import java.io.IOException; -import java.security.MessageDigest; -import java.text.MessageFormat; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -59,7 +56,6 @@ import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.errors.StoredObjectRepresentationNotAvailableException; -import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.pack.CachedPack; import org.eclipse.jgit.internal.storage.pack.ObjectReuseAsIs; import org.eclipse.jgit.internal.storage.pack.ObjectToPack; @@ -232,27 +228,13 @@ int copy(final PackFile pack, long position, final byte[] dstbuf, return cnt - need; } - public void copyPackAsIs(PackOutputStream out, CachedPack pack, - boolean validate) throws IOException { - ((LocalCachedPack) pack).copyAsIs(out, validate, this); + public void copyPackAsIs(PackOutputStream out, CachedPack pack) + throws IOException { + ((LocalCachedPack) pack).copyAsIs(out, this); } - void copyPackAsIs(final PackFile pack, final long length, boolean validate, + void copyPackAsIs(final PackFile pack, final long length, final PackOutputStream out) throws IOException { - MessageDigest md = null; - if (validate) { - md = Constants.newMessageDigest(); - byte[] buf = out.getCopyBuffer(); - pin(pack, 0); - if (window.copy(0, buf, 0, 12) != 12) { - pack.setInvalid(); - throw new IOException(MessageFormat.format( - JGitText.get().packfileIsTruncated, pack.getPackFile() - .getPath())); - } - md.update(buf, 0, 12); - } - long position = 12; long remaining = length - (12 + 20); while (0 < remaining) { @@ -260,29 +242,10 @@ void copyPackAsIs(final PackFile pack, final long length, boolean validate, int ptr = (int) (position - window.start); int n = (int) Math.min(window.size() - ptr, remaining); - window.write(out, position, n, md); + window.write(out, position, n); position += n; remaining -= n; } - - if (md != null) { - byte[] buf = new byte[20]; - byte[] actHash = md.digest(); - - pin(pack, position); - if (window.copy(position, buf, 0, 20) != 20) { - pack.setInvalid(); - throw new IOException(MessageFormat.format( - JGitText.get().packfileIsTruncated, pack.getPackFile() - .getPath())); - } - if (!Arrays.equals(actHash, buf)) { - pack.setInvalid(); - throw new IOException(MessageFormat.format( - JGitText.get().packfileCorruptionDetected, pack - .getPackFile().getPath())); - } - } } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java index 00b6b6536..2e5d59960 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java @@ -209,16 +209,11 @@ public void copyObjectAsIs(PackOutputStream out, ObjectToPack otp, * stream to append the pack onto. * @param pack * the cached pack to send. - * @param validate - * if true the representation must be validated and not be - * corrupt before being reused. If false, validation may be - * skipped as it will be performed elsewhere in the processing - * pipeline. * @throws IOException * the pack cannot be read, or stream did not accept a write. */ - public abstract void copyPackAsIs(PackOutputStream out, CachedPack pack, - boolean validate) throws IOException; + public abstract void copyPackAsIs(PackOutputStream out, CachedPack pack) + throws IOException; /** * Obtain the available cached packs that match the bitmap and update diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java index 510538d86..6d0c8e6cd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java @@ -1048,7 +1048,7 @@ public void writePack(ProgressMonitor compressMonitor, stats.reusedObjects += pack.getObjectCount(); stats.reusedDeltas += deltaCnt; stats.totalDeltas += deltaCnt; - reuseSupport.copyPackAsIs(out, pack, reuseValidate); + reuseSupport.copyPackAsIs(out, pack); } writeChecksum(out); out.flush(); @@ -1866,7 +1866,7 @@ private void findObjectsToPackUsingBitmaps( false); BitmapBuilder needBitmap = wantBitmap.andNot(haveBitmap); - if (useCachedPacks && reuseSupport != null + if (useCachedPacks && reuseSupport != null && !reuseValidate && (excludeInPacks == null || excludeInPacks.length == 0)) cachedPacks.addAll( reuseSupport.getCachedPacksAndUpdate(needBitmap));