Changeset e117e33

Timestamp:

Dec 11, 2011 10:59:37 PM (9 years ago)

Author:

zzz <zzz@…>

Branches:

master

Children:

1e5afa85

Parents:

8448001

Message:

• ReusableGZIPInputStream: Fix 3 major bugs, all present since 2005:
  • Payloads an exact multiple of 512 bytes failed to decompress
  • Data at the end of the stream could be lost
  • read() returned -1 when the data was 0xff

Files:

• core/java/src/net/i2p/util/LookaheadInputStream.java (modified) (3 diffs)
• core/java/src/net/i2p/util/ResettableGZIPInputStream.java (modified) (8 diffs)
• history.txt (modified) (1 diff)
• router/java/src/net/i2p/router/RouterVersion.java (modified) (1 diff)

core/java/src/net/i2p/util/LookaheadInputStream.java

@@ -18 +18 @@
     private static final InputStream _fakeInputStream = new ByteArrayInputStream(new byte[0]);
     
+    /**
+     *  Configure a stream that hides a number of bytes from the reader.
+     *  The last n bytes will never be available from read(),
+     *  they can only be obtained from getFooter().
+     *
+     *  initialize() MUST be called before doing any read() calls.
+     *
+     *  @param lookaheadSize how many bytes to hide
+     */
     public LookaheadInputStream(int lookaheadSize) {
         super(_fakeInputStream);
@@ -25 +34 @@
     public boolean getEOFReached() { return _eofReached; }
         
-    /** blocking! */
+    /**
+     *  Start the LookaheadInputStream with the given input stream.
+     *  Resets everything if the LookaheadInputStream was previously used.
+     *  WARNING - blocking until lookaheadSize bytes are read!
+     */
     public void initialize(InputStream src) throws IOException {
         in = src;
@@ -48 +61 @@
         }
         int rv = _footerLookahead[0];
+        // FIXME use an index!!!!!!!!!!!!
         System.arraycopy(_footerLookahead, 1, _footerLookahead, 0, _footerLookahead.length-1);
         _footerLookahead[_footerLookahead.length-1] = (byte)c;

core/java/src/net/i2p/util/ResettableGZIPInputStream.java

@@ -2 +2 @@
 
 import java.io.ByteArrayInputStream;
+import java.io.FilterInputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -20 +21 @@
     private static final int FOOTER_SIZE = 8; // CRC32 + ISIZE
     private static final boolean DEBUG = false;
-    /** keep a typesafe copy of (LookaheadInputStream)in */
+    /** See below for why this is necessary */
+    private final ExtraByteInputStream _extraByteInputStream;
+    /** keep a typesafe copy of this */
     private final LookaheadInputStream _lookaheadStream;
+    private static final byte[] _oneDummyByte = new byte[1];
+    private final InputStream _sequenceStream = null;
     private final CRC32 _crc32;
     private final byte _buf1[] = new byte[1];
@@ -32 +37 @@
      */
     public ResettableGZIPInputStream() {
-        super(new LookaheadInputStream(FOOTER_SIZE), new Inflater(true));
-        _lookaheadStream = (LookaheadInputStream)in;
+        // compressedStream -> 
+        //   LookaheadInputStream that removes last 8 bytes ->
+        //     ExtraByteInputStream that adds 1 byte ->
+        //       InflaterInputStream
+        // See below for why this is necessary
+        super(new ExtraByteInputStream(new LookaheadInputStream(FOOTER_SIZE)),
+              new Inflater(true));
+        _extraByteInputStream = (ExtraByteInputStream)in;
+        _lookaheadStream = (LookaheadInputStream)_extraByteInputStream.getInputStream();
         _crc32 = new CRC32();
     }
@@ -56 +68 @@
         _crc32.reset();
         _buf1[0] = 0x0;
+        _extraByteInputStream.reset();
         // blocking call to read the footer/lookahead, and use the compressed
         // stream as the source for further lookahead bytes
@@ -66 +79 @@
     @Override
     public int read() throws IOException {
-        if (_complete) {
-            // shortcircuit so the inflater doesn't try to refill 
-            // with the footer's data (which would fail, causing ZLIB err)
-            return -1;
-        }
         int read = read(_buf1, 0, 1);
         if (read == -1)
             return -1;
-        else
-            return _buf1[0];
+        return _buf1[0] & 0xff;
     }
     
@@ -83 +90 @@
     }
 
+    /**
+     *
+     */
     @Override
     public int read(byte buf[], int off, int len) throws IOException {
@@ -96 +106 @@
         } else {
             _crc32.update(buf, off, read);
-            if (_lookaheadStream.getEOFReached()) {
+            // NO, we can't do use getEOFReached here
+            // 1) Just because the lookahead stream has hit EOF doesn't mean
+            //    that the inflater has given us all the data yet,
+            //    this would cause data loss at the end
+            //if (_lookaheadStream.getEOFReached()) {
+            if (inf.finished()) {
                 verifyFooter();
                 inf.reset(); // so it doesn't bitch about missing data...
@@ -273 +288 @@
             c = in.read();
             if (c == -1) throw new IOException("EOF reading the CRC16");
+        }
+    }
+    
+    /**
+     *  Essentially a SequenceInputStream(in, new ByteArrayInputStream(new byte[1])),
+     *  except that this is resettable.
+     *
+     *  Unsupported:
+     *    - available() doesn't include the extra byte
+     *    - skip() doesn't skip the extra byte
+     *
+     *  Why? otherwise the inflater finished() is wrong when the compressed payload
+     *  (in between the 10 byte header and the 8 byte footer) is a multiple of 512 bytes,
+     *  which caused read(buf, off, len) above to fail.
+     *  Happened every time with 1042 byte compressed router infos, for example.
+     *
+     *  Details:
+     *
+     *  Warning with Inflater nowrap = true:
+     *
+     *     "Note: When using the 'nowrap' option it is also necessary to provide an extra "dummy" byte as input.
+     *      This is required by the ZLIB native library in order to support certain optimizations."
+     *
+     *     http://docs.oracle.com/javase/1.5.0/docs/api/java/util/zip/Inflater.html
+     *
+     *  This is for sure:
+     *
+     *     "This is not nearly specific enough to be useful.  Where in the compressed byte array is the
+     *      extra 'dummy' byte" expected?  What is it to contain? When calling setInput() is the 'len'
+     *      argument incremented to include the dummy byte or not?"
+     *
+     *     http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4795299
+     *
+     *  This is useless:
+     *
+     *     http://www.java-forums.org/new-java/38604-decompress-un-gzip-byte.html
+     *
+     *  This seems to be the definitive answer:
+     *
+     *     "The fix simply involves copying the byte array and tacking a single null byte on to the end."
+     *
+     *     http://code.google.com/p/google-apps-sso-sample/issues/detail?id=8
+     *
+     */
+    private static class ExtraByteInputStream extends FilterInputStream {
+        private static final byte DUMMY = 0;
+        private boolean _extraSent;
+
+        public ExtraByteInputStream(InputStream in) {
+            super(in);
+        }
+
+        @Override
+        public int read() throws IOException {
+            if (_extraSent)
+                return -1;
+            int rv = in.read();
+            if (rv >= 0)
+                return rv;
+            _extraSent = true;
+            return DUMMY;
+        }
+
+        @Override
+        public int read(byte buf[], int off, int len) throws IOException {
+            if (len == 0)
+                return 0;
+            if (_extraSent)
+                return -1;
+            int rv = in.read(buf, off, len);
+            if (rv >= 0)
+                return rv;
+            _extraSent = true;
+            buf[off] = DUMMY;
+            return 1;
+        }
+
+        @Override
+        public void close() throws IOException {
+            _extraSent = false;
+            in.close();
+        }
+
+        /** does NOT call in.reset() */
+        @Override
+        public void reset() {
+            _extraSent = false;
+        }
+
+        public InputStream getInputStream() {
+            return in;
         }
     }

history.txt

@@ +1 @@
+2011-12-12 zzz
+  * ExploreJob: Tweaks to handle DatabaseLookupMessage changes
+  * I2NP:
+    - Deprecate unused stream methods and I2NPMessageReader since
+      all transports provide encapsulation.
+    - Don't throw IOE from byte array methods
+    - Use cached null cert in GarlicClove
+    - Add method to limit size of buffer to read
+    - Don't check checksum at input, in most cases
+    - Reuse checksum at output, for unomodified pass-through messages
+      (but recalculating it now and logging on a mismatch for testing)
+    - Fix DatabaseLookupMessage to internally store the don't include peers as
+      a List, not a Set, so it doesn't get reordered and break the checksum
+    - Log cleanup
+  * NTCP:
+    - Zero-copy and limit size when handing buffer to I2NP
+    - Log hex dump message on I2NPMessageException, like in SSU
+    - Don't close connection on I2NPMessageException
+  * PortMapper: New service for registering application ports in the context
+  * ReusableGZIPInputStream: Fix 3 major bugs, all present since 2005:
+    - Payloads an exact multiple of 512 bytes failed to decompress
+    - Data at the end of the stream could be lost
+    - read() returned -1 when the data was 0xff
+  * SearchState: generics and cleanups
+
 2011-12-11 sponge
   * HTTP server tunnel, use log.WARN for 3 first minutes. (closes #460)

router/java/src/net/i2p/router/RouterVersion.java

@@ -19 +19 @@
     public final static String ID = "Monotone";
     public final static String VERSION = CoreVersion.VERSION;
-    public final static long BUILD = 19;
+    public final static long BUILD = 20;
 
     /** for example "-test" */