From 0b46e70155e1a14edc77f32e030094fb499887cf Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 2 Jul 2010 18:21:55 -0700 Subject: [PATCH] Fix infinite loop in IndexPack A programming error using the Inflater API led to an infinite loop within IndexPack, caused by the Inflater returning 0 from the inflate() method, but it didn't want more input. This happens when it has reached the end of the stream, or has reached a spot asking for an external dictionary. Such a case is a failure for us, and we should abort out. Thanks to Alex for pointing out that we had 3 implementations of the inflate rountine, which should be consolidated into one and use a switch to determine where to load data from. Bug: 317416 Change-Id: I34120482375b687ea36ed9154002d77047e94b1f Signed-off-by: Shawn O. Pearce --- .../org/eclipse/jgit/JGitText.properties | 1 + .../src/org/eclipse/jgit/JGitText.java | 1 + .../org/eclipse/jgit/transport/IndexPack.java | 248 +++++++----------- 3 files changed, 104 insertions(+), 146 deletions(-) diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/JGitText.properties index 4a54089e0..e4a603cbb 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/JGitText.properties @@ -357,6 +357,7 @@ unknownIndexVersionOrCorruptIndex=Unknown index version (or corrupt index): {0} unknownObjectType=Unknown object type {0}. unknownRepositoryFormat2=Unknown repository format "{0}"; expected "0". unknownRepositoryFormat=Unknown repository format +unknownZlibError=Unknown zlib error. unmergedPath=Unmerged path: {0} unpackError=unpack error {0} unreadablePackIndex=Unreadable pack index: {0} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/JGitText.java index f0bedfa2f..94cf1e48b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/JGitText.java @@ -416,6 +416,7 @@ public class JGitText extends TranslationBundle { /***/ public String unknownObjectType; /***/ public String unknownRepositoryFormat2; /***/ public String unknownRepositoryFormat; + /***/ public String unknownZlibError; /***/ public String unmergedPath; /***/ public String unpackError; /***/ public String unreadablePackIndex; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java index 2a5b4344f..789d8cdd7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java @@ -131,6 +131,19 @@ public class IndexPack { return ip; } + private static enum Source { + /** Data is read from the incoming stream. */ + INPUT, + + /** + * Data is read from the spooled pack file. + *

+ * During streaming, some (or all) data might be saved into the spooled + * pack file so it can be randomly accessed later. + */ + FILE; + } + private final Repository repo; /** @@ -200,7 +213,7 @@ public class IndexPack { private LongMap baseByPos; - private byte[] objectData; + private byte[] skipBuffer; private MessageDigest packDigest; @@ -232,7 +245,7 @@ public class IndexPack { inflater = InflaterCache.get(); readCurs = new WindowCursor(); buf = new byte[BUFFER_SIZE]; - objectData = new byte[BUFFER_SIZE]; + skipBuffer = new byte[512]; objectDigest = Constants.newMessageDigest(); tempObjectId = new MutableObjectId(); packDigest = Constants.newMessageDigest(); @@ -474,12 +487,12 @@ public class IndexPack { byte[] data, PackedObjectInfo oe) throws IOException { crc.reset(); position(pos); - int c = readFromFile(); + int c = readFrom(Source.FILE); final int typeCode = (c >> 4) & 7; long sz = c & 15; int shift = 4; while ((c & 0x80) != 0) { - c = readFromFile(); + c = readFrom(Source.FILE); sz += (c & 0x7f) << shift; shift += 7; } @@ -490,19 +503,19 @@ public class IndexPack { case Constants.OBJ_BLOB: case Constants.OBJ_TAG: type = typeCode; - data = inflateFromFile((int) sz); + data = inflateAndReturn(Source.FILE, sz); break; case Constants.OBJ_OFS_DELTA: { - c = readFromFile() & 0xff; + c = readFrom(Source.FILE) & 0xff; while ((c & 128) != 0) - c = readFromFile() & 0xff; - data = BinaryDelta.apply(data, inflateFromFile((int) sz)); + c = readFrom(Source.FILE) & 0xff; + data = BinaryDelta.apply(data, inflateAndReturn(Source.FILE, sz)); break; } case Constants.OBJ_REF_DELTA: { - crc.update(buf, fillFromFile(20), 20); + crc.update(buf, fill(Source.FILE, 20), 20); use(20); - data = BinaryDelta.apply(data, inflateFromFile((int) sz)); + data = BinaryDelta.apply(data, inflateAndReturn(Source.FILE, sz)); break; } default: @@ -657,7 +670,7 @@ public class IndexPack { packOut.seek(0); bAvail = 0; bOffset = 0; - fillFromFile(12); + fill(Source.FILE, 12); { final int origCnt = (int) Math.min(bAvail, origRemaining); @@ -728,7 +741,7 @@ public class IndexPack { private void readPackHeader() throws IOException { final int hdrln = Constants.PACK_SIGNATURE.length + 4 + 4; - final int p = fillFromInput(hdrln); + final int p = fill(Source.INPUT, hdrln); for (int k = 0; k < Constants.PACK_SIGNATURE.length; k++) if (buf[p + k] != Constants.PACK_SIGNATURE[k]) throw new IOException(JGitText.get().notAPACKFile); @@ -743,7 +756,7 @@ public class IndexPack { private void readPackFooter() throws IOException { sync(); final byte[] cmpcsum = packDigest.digest(); - final int c = fillFromInput(20); + final int c = fill(Source.INPUT, 20); packcsum = new byte[20]; System.arraycopy(buf, c, packcsum, 0, 20); use(20); @@ -757,7 +770,7 @@ public class IndexPack { // Cleanup all resources associated with our input parsing. private void endInput() { in = null; - objectData = null; + skipBuffer = null; } // Read one entire object or delta from the input. @@ -765,12 +778,12 @@ public class IndexPack { final long pos = position(); crc.reset(); - int c = readFromInput(); + int c = readFrom(Source.INPUT); final int typeCode = (c >> 4) & 7; long sz = c & 15; int shift = 4; while ((c & 0x80) != 0) { - c = readFromInput(); + c = readFrom(Source.INPUT); sz += (c & 0x7f) << shift; shift += 7; } @@ -783,24 +796,24 @@ public class IndexPack { whole(typeCode, pos, sz); break; case Constants.OBJ_OFS_DELTA: { - c = readFromInput(); + c = readFrom(Source.INPUT); long ofs = c & 127; while ((c & 128) != 0) { ofs += 1; - c = readFromInput(); + c = readFrom(Source.INPUT); ofs <<= 7; ofs += (c & 127); } final long base = pos - ofs; final UnresolvedDelta n; - skipInflateFromInput(sz); + inflateAndSkip(Source.INPUT, sz); n = new UnresolvedDelta(pos, (int) crc.getValue()); n.next = baseByPos.put(base, n); deltaCount++; break; } case Constants.OBJ_REF_DELTA: { - c = fillFromInput(20); + c = fill(Source.INPUT, 20); crc.update(buf, c, 20); final ObjectId base = ObjectId.fromRaw(buf, c); use(20); @@ -809,7 +822,7 @@ public class IndexPack { r = new DeltaChain(base); baseById.add(r); } - skipInflateFromInput(sz); + inflateAndSkip(Source.INPUT, sz); r.add(new UnresolvedDelta(pos, (int) crc.getValue())); deltaCount++; break; @@ -821,7 +834,7 @@ public class IndexPack { private void whole(final int type, final long pos, final long sz) throws IOException { - final byte[] data = inflateFromInput(sz); + final byte[] data = inflateAndReturn(Source.INPUT, sz); objectDigest.update(Constants.encodedTypeString(type)); objectDigest.update((byte) ' '); objectDigest.update(Constants.encodeASCII(sz)); @@ -867,19 +880,9 @@ public class IndexPack { } // Consume exactly one byte from the buffer and return it. - private int readFromInput() throws IOException { - if (bAvail == 0) - fillFromInput(1); - bAvail--; - final int b = buf[bOffset++] & 0xff; - crc.update(b); - return b; - } - - // Consume exactly one byte from the buffer and return it. - private int readFromFile() throws IOException { + private int readFrom(final Source src) throws IOException { if (bAvail == 0) - fillFromFile(1); + fill(src, 1); bAvail--; final int b = buf[bOffset++] & 0xff; crc.update(b); @@ -893,36 +896,32 @@ public class IndexPack { } // Ensure at least need bytes are available in in {@link #buf}. - private int fillFromInput(final int need) throws IOException { + private int fill(final Source src, final int need) throws IOException { while (bAvail < need) { int next = bOffset + bAvail; int free = buf.length - next; if (free + bAvail < need) { - sync(); + switch(src){ + case INPUT: + sync(); + break; + case FILE: + if (bAvail > 0) + System.arraycopy(buf, bOffset, buf, 0, bAvail); + bOffset = 0; + break; + } next = bAvail; free = buf.length - next; } - next = in.read(buf, next, free); - if (next <= 0) - throw new EOFException(JGitText.get().packfileIsTruncated); - bAvail += next; - } - return bOffset; - } - - // Ensure at least need bytes are available in in {@link #buf}. - private int fillFromFile(final int need) throws IOException { - if (bAvail < need) { - int next = bOffset + bAvail; - int free = buf.length - next; - if (free + bAvail < need) { - if (bAvail > 0) - System.arraycopy(buf, bOffset, buf, 0, bAvail); - bOffset = 0; - next = bAvail; - free = buf.length - next; + switch(src){ + case INPUT: + next = in.read(buf, next, free); + break; + case FILE: + next = packOut.read(buf, next, free); + break; } - next = packOut.read(buf, next, free); if (next <= 0) throw new EOFException(JGitText.get().packfileIsTruncated); bAvail += next; @@ -941,112 +940,69 @@ public class IndexPack { bOffset = 0; } - private void skipInflateFromInput(long sz) throws IOException { - final Inflater inf = inflater; - try { - final byte[] dst = objectData; - int n = 0; - int p = -1; - while (!inf.finished()) { - if (inf.needsInput()) { - if (p >= 0) { - crc.update(buf, p, bAvail); - use(bAvail); - } - p = fillFromInput(1); - inf.setInput(buf, p, bAvail); - } - - int free = dst.length - n; - if (free < 8) { - sz -= n; - n = 0; - free = dst.length; - } - n += inf.inflate(dst, n, free); - } - if (n != sz) - throw new DataFormatException(JGitText.get().wrongDecompressedLength); - n = bAvail - inf.getRemaining(); - if (n > 0) { - crc.update(buf, p, n); - use(n); - } - } catch (DataFormatException dfe) { - throw corrupt(dfe); - } finally { - inf.reset(); - } + private void inflateAndSkip(final Source src, final long inflatedSize) + throws IOException { + inflate(src, inflatedSize, skipBuffer, false /* do not keep result */); } - private byte[] inflateFromInput(final long sz) throws IOException { - final byte[] dst = new byte[(int) sz]; - final Inflater inf = inflater; - try { - int n = 0; - int p = -1; - while (!inf.finished()) { - if (inf.needsInput()) { - if (p >= 0) { - crc.update(buf, p, bAvail); - use(bAvail); - } - p = fillFromInput(1); - inf.setInput(buf, p, bAvail); - } - - n += inf.inflate(dst, n, dst.length - n); - } - if (n != sz) - throw new DataFormatException(JGitText.get().wrongDecompressedLength); - n = bAvail - inf.getRemaining(); - if (n > 0) { - crc.update(buf, p, n); - use(n); - } - return dst; - } catch (DataFormatException dfe) { - throw corrupt(dfe); - } finally { - inf.reset(); - } + private byte[] inflateAndReturn(final Source src, final long inflatedSize) + throws IOException { + final byte[] dst = new byte[(int) inflatedSize]; + inflate(src, inflatedSize, dst, true /* keep result in dst */); + return dst; } - private byte[] inflateFromFile(final int sz) throws IOException { + private void inflate(final Source src, final long inflatedSize, + final byte[] dst, final boolean keep) throws IOException { final Inflater inf = inflater; try { - final byte[] dst = new byte[sz]; - int n = 0; - int p = -1; - while (!inf.finished()) { - if (inf.needsInput()) { - if (p >= 0) { - crc.update(buf, p, bAvail); - use(bAvail); + int off = 0; + long cnt = 0; + int p = fill(src, 24); + inf.setInput(buf, p, bAvail); + + do { + int r = inf.inflate(dst, off, dst.length - off); + if (r == 0) { + if (inf.finished()) + break; + if (inf.needsInput()) { + if (p >= 0) { + crc.update(buf, p, bAvail); + use(bAvail); + } + p = fill(src, 24); + inf.setInput(buf, p, bAvail); + } else { + throw new CorruptObjectException(MessageFormat.format( + JGitText.get().packfileCorruptionDetected, + JGitText.get().unknownZlibError)); } - p = fillFromFile(1); - inf.setInput(buf, p, bAvail); } - n += inf.inflate(dst, n, sz - n); + cnt += r; + if (keep) + off += r; + } while (cnt < inflatedSize); + + if (!inf.finished() || cnt != inflatedSize) { + throw new CorruptObjectException(MessageFormat.format(JGitText + .get().packfileCorruptionDetected, + JGitText.get().wrongDecompressedLength)); } - n = bAvail - inf.getRemaining(); - if (n > 0) { - crc.update(buf, p, n); - use(n); + + int left = bAvail - inf.getRemaining(); + if (left > 0) { + crc.update(buf, p, left); + use(left); } - return dst; } catch (DataFormatException dfe) { - throw corrupt(dfe); + throw new CorruptObjectException(MessageFormat.format(JGitText + .get().packfileCorruptionDetected, dfe.getMessage())); } finally { inf.reset(); } } - private static CorruptObjectException corrupt(final DataFormatException dfe) { - return new CorruptObjectException(MessageFormat.format( - JGitText.get().packfileCorruptionDetected, dfe.getMessage())); - } - private static class DeltaChain extends ObjectId { UnresolvedDelta head;