Fix chunkedCoding. (#9)

I misread the spec; it turns out there's a CR LF sequence after the body
bytes as well as after the size header.

CHANGELOG.md

@@ -1,3 +1,8 @@
+## 3.1.1
+
+* Fix a logic bug in the `chunkedCoding` codec. It had been producing invalid
+  output and rejecting valid input.
+
 ## 3.1.0
 
 * Add `chunkedCoding`, a `Codec` that supports encoding and decoding the

lib/src/chunked_coding/decoder.dart

@@ -70,7 +70,7 @@ class _Sink extends ByteConversionSinkBase {
     /// describe the character in the exception text.
     assertCurrentChar(int char, String name) {
       if (bytes[start] != char) {
-        throw new FormatException("Expected LF.", bytes, start);
+        throw new FormatException("Expected $name.", bytes, start);
       }
     }
 
@@ -85,7 +85,7 @@ class _Sink extends ByteConversionSinkBase {
 
         case _State.size:
           if (bytes[start] == $cr) {
-            _state = _State.beforeLF;
+            _state = _State.sizeBeforeLF;
           } else {
             // Shift four bits left since a single hex digit contains four bits
             // of information.
@@ -94,7 +94,7 @@ class _Sink extends ByteConversionSinkBase {
           start++;
           break;
 
-        case _State.beforeLF:
+        case _State.sizeBeforeLF:
           assertCurrentChar($lf, "LF");
           _state = _size == 0 ? _State.endBeforeCR : _State.body;
           start++;
@@ -105,7 +105,19 @@ class _Sink extends ByteConversionSinkBase {
           buffer.addAll(bytes, start, chunkEnd);
           _size -= chunkEnd - start;
           start = chunkEnd;
-          if (_size == 0) _state = _State.boundary;
+          if (_size == 0) _state = _State.bodyBeforeCR;
+          break;
+
+        case _State.bodyBeforeCR:
+          assertCurrentChar($cr, "CR");
+          _state = _State.bodyBeforeLF;
+          start++;
+          break;
+
+        case _State.bodyBeforeLF:
+          assertCurrentChar($lf, "LF");
+          _state = _State.boundary;
+          start++;
           break;
 
         case _State.endBeforeCR:
@@ -115,7 +127,7 @@ class _Sink extends ByteConversionSinkBase {
           break;
 
         case _State.endBeforeLF:
-          assertCurrentChar($lf, "CR");
+          assertCurrentChar($lf, "LF");
           _state = _State.end;
           start++;
           break;
@@ -161,8 +173,6 @@ class _Sink extends ByteConversionSinkBase {
 
 /// An enumeration of states that [_Sink] can exist in when decoded a chunked
 /// message.
-///
-/// [_SizeState], [_CRState], and [_ChunkState] have additional data attached.
 class _State {
   /// The parser has fully parsed one chunk and is expecting the header for the
   /// next chunk.
@@ -173,20 +183,32 @@ class _State {
   /// The parser has parsed at least one digit of the chunk size header, but has
   /// not yet parsed the `CR LF` sequence that indicates the end of that header.
   ///
-  /// Transitions to [beforeLF].
+  /// Transitions to [sizeBeforeLF].
   static const size = const _State._("size");
 
   /// The parser has parsed the chunk size header and the CR character after it,
   /// but not the LF.
   ///
-  /// Transitions to [body] or [endBeforeCR].
-  static const beforeLF = const _State._("before LF");
+  /// Transitions to [body] or [bodyBeforeCR].
+  static const sizeBeforeLF = const _State._("size before LF");
 
   /// The parser has parsed a chunk header and possibly some of the body, but
   /// still needs to consume more bytes.
   ///
-  /// Transitions to [boundary].
-  static const body = const _State._("CR");
+  /// Transitions to [bodyBeforeCR].
+  static const body = const _State._("body");
+
+  // The parser has parsed all the bytes in a chunk body but not the CR LF
+  // sequence that follows it.
+  //
+  // Transitions to [bodyBeforeLF].
+  static const bodyBeforeCR = const _State._("body before CR");
+
+  // The parser has parsed all the bytes in a chunk body and the CR that follows
+  // it, but not the LF after that.
+  //
+  // Transitions to [bounday].
+  static const bodyBeforeLF = const _State._("body before LF");
 
   /// The parser has parsed the final empty chunk but not the CR LF sequence
   /// that follows it.

lib/src/chunked_coding/encoder.dart

@@ -59,12 +59,18 @@ List<int> _convert(List<int> bytes, int start, int end, {bool isLast: false}) {
   var sizeInHex = size.toRadixString(16);
   var footerSize = isLast ? _doneChunk.length : 0;
 
-  // Add 2 for the CRLF sequence that follows the size header.
-  var list = new Uint8List(sizeInHex.length + 2 + size + footerSize);
+  // Add 4 for the CRLF sequences that follow the size header and the bytes.
+  var list = new Uint8List(sizeInHex.length + 4 + size + footerSize);
   list.setRange(0, sizeInHex.length, sizeInHex.codeUnits);
-  list[sizeInHex.length] = $cr;
-  list[sizeInHex.length + 1] = $lf;
-  list.setRange(sizeInHex.length + 2, list.length - footerSize, bytes, start);
+
+  var cursor = sizeInHex.length;
+  list[cursor++] = $cr;
+  list[cursor++] = $lf;
+  list.setRange(cursor, cursor + end - start, bytes, start);
+  cursor += end - start;
+  list[cursor++] = $cr;
+  list[cursor++] = $lf;
+
   if (isLast) {
     list.setRange(list.length - footerSize, list.length, _doneChunk);
   }

test/chunked_coding_test.dart

@@ -14,15 +14,15 @@ void main() {
   group("encoder", () {
     test("adds a header to the chunk of bytes", () {
       expect(chunkedCoding.encode([1, 2, 3]),
-          equals([$3, $cr, $lf, 1, 2, 3, $0, $cr, $lf, $cr, $lf]));
+          equals([$3, $cr, $lf, 1, 2, 3, $cr, $lf, $0, $cr, $lf, $cr, $lf]));
     });
 
     test("uses hex for chunk size", () {
       var data = new Iterable.generate(0xA7).toList();
       expect(chunkedCoding.encode(data),
           equals([$a, $7, $cr, $lf]
             ..addAll(data)
-            ..addAll([$0, $cr, $lf, $cr, $lf])));
+            ..addAll([$cr, $lf, $0, $cr, $lf, $cr, $lf])));
     });
 
     test("just generates a footer for an empty input", () {
@@ -41,18 +41,18 @@ void main() {
 
       test("adds headers to each chunk of bytes", () {
         sink.add([1, 2, 3, 4]);
-        expect(results, equals([[$4, $cr, $lf, 1, 2, 3, 4]]));
+        expect(results, equals([[$4, $cr, $lf, 1, 2, 3, 4, $cr, $lf]]));
 
         sink.add([5, 6, 7]);
         expect(results, equals([
-          [$4, $cr, $lf, 1, 2, 3, 4],
-          [$3, $cr, $lf, 5, 6, 7],
+          [$4, $cr, $lf, 1, 2, 3, 4, $cr, $lf],
+          [$3, $cr, $lf, 5, 6, 7, $cr, $lf],
         ]));
 
         sink.close();
         expect(results, equals([
-          [$4, $cr, $lf, 1, 2, 3, 4],
-          [$3, $cr, $lf, 5, 6, 7],
+          [$4, $cr, $lf, 1, 2, 3, 4, $cr, $lf],
+          [$3, $cr, $lf, 5, 6, 7, $cr, $lf],
           [$0, $cr, $lf, $cr, $lf],
         ]));
       });
@@ -62,15 +62,15 @@ void main() {
         expect(results, equals([[]]));
 
         sink.add([1, 2, 3]);
-        expect(results, equals([[], [$3, $cr, $lf, 1, 2, 3]]));
+        expect(results, equals([[], [$3, $cr, $lf, 1, 2, 3, $cr, $lf]]));
 
         sink.add([]);
-        expect(results, equals([[], [$3, $cr, $lf, 1, 2, 3], []]));
+        expect(results, equals([[], [$3, $cr, $lf, 1, 2, 3, $cr, $lf], []]));
 
         sink.close();
         expect(results, equals([
           [],
-          [$3, $cr, $lf, 1, 2, 3],
+          [$3, $cr, $lf, 1, 2, 3, $cr, $lf],
           [],
           [$0, $cr, $lf, $cr, $lf],
         ]));
@@ -79,7 +79,7 @@ void main() {
       group("addSlice()", () {
         test("adds bytes from the specified slice", () {
           sink.addSlice([1, 2, 3, 4, 5], 1, 4, false);
-          expect(results, equals([[$3, $cr, $lf, 2, 3, 4]]));
+          expect(results, equals([[$3, $cr, $lf, 2, 3, 4, $cr, $lf]]));
         });
 
         test("doesn't add a header if the slice is empty", () {
@@ -89,8 +89,8 @@ void main() {
 
         test("adds a footer if isLast is true", () {
           sink.addSlice([1, 2, 3, 4, 5], 1, 4, true);
-          expect(results,
-              equals([[$3, $cr, $lf, 2, 3, 4, $0, $cr, $lf, $cr, $lf]]));
+          expect(results, equals(
+              [[$3, $cr, $lf, 2, 3, 4, $cr, $lf, $0, $cr, $lf, $cr, $lf]]));
 
           // Setting isLast shuld close the sink.
           expect(() => sink.add([]), throwsStateError);
@@ -119,8 +119,8 @@ void main() {
   group("decoder", () {
     test("parses chunked data", () {
       expect(chunkedCoding.decode([
-        $3, $cr, $lf, 1, 2, 3,
-        $4, $cr, $lf, 4, 5, 6, 7,
+        $3, $cr, $lf, 1, 2, 3, $cr, $lf,
+        $4, $cr, $lf, 4, 5, 6, 7, $cr, $lf,
         $0, $cr, $lf, $cr, $lf,
       ]), equals([1, 2, 3, 4, 5, 6, 7]));
     });
@@ -130,7 +130,7 @@ void main() {
       expect(
           chunkedCoding.decode([$a, $7, $cr, $lf]
             ..addAll(data)
-            ..addAll([$0, $cr, $lf, $cr, $lf])),
+            ..addAll([$cr, $lf, $0, $cr, $lf, $cr, $lf])),
           equals(data));
     });
 
@@ -139,7 +139,7 @@ void main() {
       expect(
           chunkedCoding.decode([$A, $7, $cr, $lf]
             ..addAll(data)
-            ..addAll([$0, $cr, $lf, $cr, $lf])),
+            ..addAll([$cr, $lf, $0, $cr, $lf, $cr, $lf])),
           equals(data));
     });
 
@@ -170,11 +170,21 @@ void main() {
             throwsFormatException);
       });
 
-      test("that ends at a chunk boundary", () {
+      test("that ends after a chunk's bytes", () {
         expect(() => chunkedCoding.decode([$1, $cr, $lf, 1]),
             throwsFormatException);
       });
 
+      test("that ends after a chunk's CR", () {
+        expect(() => chunkedCoding.decode([$1, $cr, $lf, 1, $cr]),
+            throwsFormatException);
+      });
+
+      test("that ends atfter a chunk's LF", () {
+        expect(() => chunkedCoding.decode([$1, $cr, $lf, 1, $cr, $lf]),
+            throwsFormatException);
+      });
+
       test("that ends after the empty chunk", () {
         expect(() => chunkedCoding.decode([$0, $cr, $lf]),
             throwsFormatException);
@@ -208,10 +218,10 @@ void main() {
       });
 
       test("decodes each chunk of bytes", () {
-        sink.add([$4, $cr, $lf, 1, 2, 3, 4]);
+        sink.add([$4, $cr, $lf, 1, 2, 3, 4, $cr, $lf]);
         expect(results, equals([[1, 2, 3, 4]]));
 
-        sink.add([$3, $cr, $lf, 5, 6, 7]);
+        sink.add([$3, $cr, $lf, 5, 6, 7, $cr, $lf]);
         expect(results, equals([[1, 2, 3, 4], [5, 6, 7]]));
 
         sink.add([$0, $cr, $lf, $cr, $lf]);
@@ -223,7 +233,7 @@ void main() {
         sink.add([]);
         expect(results, isEmpty);
 
-        sink.add([$3, $cr, $lf, 1, 2, 3]);
+        sink.add([$3, $cr, $lf, 1, 2, 3, $cr, $lf]);
         expect(results, equals([[1, 2, 3]]));
 
         sink.add([]);
@@ -281,6 +291,30 @@ void main() {
           expect(results, equals([[1, 2], [3]]));
         });
 
+        test("after all bytes", () {
+          sink.add([$3, $cr, $lf, 1, 2, 3]);
+          expect(results, equals([[1, 2, 3]]));
+
+          sink.add([$cr, $lf, $3, $cr, $lf, 2, 3, 4, $cr, $lf]);
+          expect(results, equals([[1, 2, 3], [2, 3, 4]]));
+        });
+
+        test("after a post-chunk CR", () {
+          sink.add([$3, $cr, $lf, 1, 2, 3, $cr]);
+          expect(results, equals([[1, 2, 3]]));
+
+          sink.add([$lf, $3, $cr, $lf, 2, 3, 4, $cr, $lf]);
+          expect(results, equals([[1, 2, 3], [2, 3, 4]]));
+        });
+
+        test("after a post-chunk LF", () {
+          sink.add([$3, $cr, $lf, 1, 2, 3, $cr, $lf]);
+          expect(results, equals([[1, 2, 3]]));
+
+          sink.add([$3, $cr, $lf, 2, 3, 4, $cr, $lf]);
+          expect(results, equals([[1, 2, 3], [2, 3, 4]]));
+        });
+
         test("after empty chunk size", () {
           sink.add([$0]);
           expect(results, isEmpty);