From 317b58a3a9cdc1a5cff1ca33b95271ca62e7f1f4 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Wed, 14 Dec 2016 17:15:34 -0800 Subject: [PATCH] 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 | 5 ++ lib/src/chunked_coding/decoder.dart | 46 ++++++++++++----- lib/src/chunked_coding/encoder.dart | 16 ++++-- test/chunked_coding_test.dart | 76 +++++++++++++++++++++-------- 4 files changed, 105 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c12d2cb..2c50995 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/src/chunked_coding/decoder.dart b/lib/src/chunked_coding/decoder.dart index e2a27fc..18530bc 100644 --- a/lib/src/chunked_coding/decoder.dart +++ b/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. diff --git a/lib/src/chunked_coding/encoder.dart b/lib/src/chunked_coding/encoder.dart index c724700..183ecfc 100644 --- a/lib/src/chunked_coding/encoder.dart +++ b/lib/src/chunked_coding/encoder.dart @@ -59,12 +59,18 @@ List _convert(List 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); } diff --git a/test/chunked_coding_test.dart b/test/chunked_coding_test.dart index 29fce25..241cfc9 100644 --- a/test/chunked_coding_test.dart +++ b/test/chunked_coding_test.dart @@ -14,7 +14,7 @@ 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", () { @@ -22,7 +22,7 @@ void main() { 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);