Cleanup use of java.util.Inflater, fixing rare infinite loops

The native implementation of inflate() can set finished to return
true at the same time as it copies the last bytes into the buffer.

Check for finished on each iteration, terminating as soon as libz
knows the stream was completely inflated.

If not finished, it is likely input is required before the next
native call could do any useful work. Most invocations are passing
in a buffer large enough to store the entire result. A partial return
from inflate() will need more input before it can continue. Checking
right away that needsInput() is true saves a native call to determine
no bytes can be inflated without more input.

This should fix a rare infinite loop condition inside of inflation
when an object ends exactly at the end of a block boundary, and
the next block contains only the 20 byte trailing SHA-1.

When the stream is finished each new attempt to inflate() returns
n == 0, as no additional bytes were output. The needsInput() test
tries to add the length of the footer block to itself, but then loops
back around an reloads the same block as the block is smaller than
a full block size. A zero length input is set to the inflater,
which triggers needsInput() condition again.

Change-Id: I95d02bfeab4bf995a254d49166b4ae62d1f21346

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 7289b9ee9cc6b1779ea43f43b01ede139067a11e..c1853327f55304a8b2920fade53c81fb3e979dda 100644 (file)
--- 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
@@ -74,11 +74,6 @@ final class DfsBlock {
                return block.length;
        }
 
-       int remaining(long pos) {
-               int ptr = (int) (pos - start);
-               return block.length - ptr;
-       }
-
        boolean contains(DfsPackKey want, long pos) {
                return pack == want && start <= pos && pos < end;
        }
@@ -94,9 +89,11 @@ final class DfsBlock {
                return n;
        }
 
-       void setInput(Inflater inf, long pos) {
+       int setInput(long pos, Inflater inf) {
                int ptr = (int) (pos - start);
-               inf.setInput(block, ptr, block.length - ptr);
+               int cnt = block.length - ptr;
+               inf.setInput(block, ptr, cnt);
+               return cnt;
        }
 
        void crc32(CRC32 out, long pos, int cnt) {

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsInserter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsInserter.java
index 22a03dd8e674eb05add62c553a893e3e217c13d5..5cab473c33036ce8c70012032229d38cfc753f64 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsInserter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsInserter.java
@@ -460,31 +460,29 @@ public class DfsInserter extends ObjectInserter {
                        }
 
                        Inflater inf = ctx.inflater();
-                       DfsBlock b = setInput(inf, pos);
+                       pos += setInput(pos, inf);
                        for (int dstoff = 0;;) {
                                int n = inf.inflate(dstbuf, dstoff, dstbuf.length - dstoff);
-                               if (n > 0)
-                                       dstoff += n;
-                               else if (inf.needsInput() && b != null) {
-                                       pos += b.remaining(pos);
-                                       b = setInput(inf, pos);
-                               } else if (inf.needsInput())
-                                       throw new EOFException(DfsText.get().unexpectedEofInPack);
-                               else if (inf.finished())
+                               dstoff += n;
+                               if (inf.finished())
                                        return dstbuf;
-                               else
+                               if (inf.needsInput())
+                                       pos += setInput(pos, inf);
+                               else if (n == 0)
                                        throw new DataFormatException();
                        }
                }
 
-               private DfsBlock setInput(Inflater inf, long pos) throws IOException {
-                       if (pos < currPos) {
-                               DfsBlock b = getOrLoadBlock(pos);
-                               b.setInput(inf, pos);
-                               return b;
+               private int setInput(long pos, Inflater inf) throws IOException {
+                       if (pos < currPos)
+                               return getOrLoadBlock(pos).setInput(pos, inf);
+                       if (pos < currPos + currPtr) {
+                               int s = (int) (pos - currPos);
+                               int n = currPtr - s;
+                               inf.setInput(currBuf, s, n);
+                               return n;
                        }
-                       inf.setInput(currBuf, (int) (pos - currPos), currPtr);
-                       return null;
+                       throw new EOFException(DfsText.get().unexpectedEofInPack);
                }
 
                private DfsBlock getOrLoadBlock(long pos) throws IOException {

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 d7b222866dc0ae72aa96f0a7ab02dc3901f073bd..37dbc7ea7e1e361e075ae71126c0fe49a1c27b2a 100644 (file)
--- 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
@@ -600,11 +600,11 @@ public final class DfsReader extends ObjectReader implements ObjectReuseAsIs {
         *            position within the file to read from.
         * @param dstbuf
         *            destination buffer the inflater should output decompressed
-        *            data to.
+        *            data to. Must be large enough to store the entire stream,
+        *            unless headerOnly is true.
         * @param headerOnly
         *            if true the caller wants only {@code dstbuf.length} bytes.
-        * @return updated <code>dstoff</code> based on the number of bytes
-        *         successfully inflated into <code>dstbuf</code>.
+        * @return number of bytes inflated into <code>dstbuf</code>.
         * @throws IOException
         *             this cursor does not match the provider or id and the proper
         *             window could not be acquired through the provider's cache.
@@ -616,24 +616,17 @@ public final class DfsReader extends ObjectReader implements ObjectReuseAsIs {
                        boolean headerOnly) throws IOException, DataFormatException {
                prepareInflater();
                pin(pack, position);
-               block.setInput(inf, position);
-               int dstoff = 0;
-               for (;;) {
+               position += block.setInput(position, inf);
+               for (int dstoff = 0;;) {
                        int n = inf.inflate(dstbuf, dstoff, dstbuf.length - dstoff);
-                       if (n == 0) {
-                               if (headerOnly && dstoff == dstbuf.length)
-                                       return dstoff;
-                               if (inf.needsInput()) {
-                                       position += block.remaining(position);
-                                       pin(pack, position);
-                                       block.setInput(inf, position);
-                                       continue;
-                               }
-                               if (inf.finished())
-                                       return dstoff;
-                               throw new DataFormatException();
-                       }
                        dstoff += n;
+                       if (inf.finished() || (headerOnly && dstoff == dstbuf.length))
+                               return dstoff;
+                       if (inf.needsInput()) {
+                               pin(pack, position);
+                               position += block.setInput(position, inf);
+                       } else if (n == 0)
+                               throw new DataFormatException();
                }
        }
 

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 81f8ab67561a83235fc54d5aaa5eba371815a6c7..306a0d3896e71d0c22f13fedb2fb49d523b328a5 100644 (file)
--- 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
@@ -335,7 +335,7 @@ public class PackFile implements Iterable<PackIndex.MutableEntry> {
                        return null;
                }
 
-               if (curs.inflate(this, position, dstbuf, 0) != sz)
+               if (curs.inflate(this, position, dstbuf, false) != sz)
                        throw new EOFException(MessageFormat.format(
                                        JGitText.get().shortCompressedStreamAt,
                                        Long.valueOf(position)));
@@ -886,7 +886,7 @@ public class PackFile implements Iterable<PackIndex.MutableEntry> {
                // the longest delta instruction header.
                //
                final byte[] hdr = new byte[18];
-               wc.inflate(this, pos, hdr, 0);
+               wc.inflate(this, pos, hdr, true /* headerOnly */);
                return hdr;
        }
 

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 8dcbeb896ef4716932a790da010756785916958c..a0a4c952fce64ffe77d16b22622435b07d4896b1 100644 (file)
--- 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
@@ -290,11 +290,11 @@ final class WindowCursor extends ObjectReader implements ObjectReuseAsIs {
         *            position within the file to read from.
         * @param dstbuf
         *            destination buffer the inflater should output decompressed
-        *            data to.
-        * @param dstoff
-        *            current offset within <code>dstbuf</code> to inflate into.
-        * @return updated <code>dstoff</code> based on the number of bytes
-        *         successfully inflated into <code>dstbuf</code>.
+        *            data to. Must be large enough to store the entire stream,
+        *            unless headerOnly is true.
+        * @param headerOnly
+        *            if true the caller wants only {@code dstbuf.length} bytes.
+        * @return number of bytes inflated into <code>dstbuf</code>.
         * @throws IOException
         *             this cursor does not match the provider or id and the proper
         *             window could not be acquired through the provider's cache.
@@ -303,24 +303,21 @@ final class WindowCursor extends ObjectReader implements ObjectReuseAsIs {
         *             stream corruption is likely.
         */
        int inflate(final PackFile pack, long position, final byte[] dstbuf,
-                       int dstoff) throws IOException, DataFormatException {
+                       boolean headerOnly) throws IOException, DataFormatException {
                prepareInflater();
                pin(pack, position);
                position += window.setInput(position, inf);
-               do {
+               for (int dstoff = 0;;) {
                        int n = inf.inflate(dstbuf, dstoff, dstbuf.length - dstoff);
-                       if (n == 0) {
-                               if (inf.needsInput()) {
-                                       pin(pack, position);
-                                       position += window.setInput(position, inf);
-                               } else if (inf.finished())
-                                       return dstoff;
-                               else
-                                       throw new DataFormatException();
-                       }
                        dstoff += n;
-               } while (dstoff < dstbuf.length);
-               return dstoff;
+                       if (inf.finished() || (headerOnly && dstoff == dstbuf.length))
+                               return dstoff;
+                       if (inf.needsInput()) {
+                               pin(pack, position);
+                               position += window.setInput(position, inf);
+                       } else if (n == 0)
+                               throw new DataFormatException();
+               }
        }
 
        ByteArrayWindow quickCopy(PackFile p, long pos, long cnt)

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java
index c3fdacc27a6ed5e9072b68f698396dab611ad36a..633554a96fa12b348ba093fbe33dd6ced1d207a2 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java
@@ -1653,24 +1653,19 @@ public abstract class PackParser {
                                int n = 0;
                                while (n < cnt) {
                                        int r = inf.inflate(dst, pos + n, cnt - n);
-                                       if (r == 0) {
-                                               if (inf.finished())
-                                                       break;
-                                               if (inf.needsInput()) {
-                                                       onObjectData(src, buf, p, bAvail);
-                                                       use(bAvail);
-
-                                                       p = fill(src, 1);
-                                                       inf.setInput(buf, p, bAvail);
-                                               } else {
-                                                       throw new CorruptObjectException(
-                                                                       MessageFormat
-                                                                                       .format(
-                                                                                                       JGitText.get().packfileCorruptionDetected,
-                                                                                                       JGitText.get().unknownZlibError));
-                                               }
-                                       } else {
-                                               n += r;
+                                       n += r;
+                                       if (inf.finished())
+                                               break;
+                                       if (inf.needsInput()) {
+                                               onObjectData(src, buf, p, bAvail);
+                                               use(bAvail);
+
+                                               p = fill(src, 1);
+                                               inf.setInput(buf, p, bAvail);
+                                       } else if (r == 0) {
+                                               throw new CorruptObjectException(MessageFormat.format(
+                                                               JGitText.get().packfileCorruptionDetected,
+                                                               JGitText.get().unknownZlibError));
                                        }
                                }
                                actualSize += n;