From 68d74fb703a6342e3c6a8f71b9e551f0d1dcaef3 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Thu, 24 Mar 2022 12:54:03 -0700 Subject: [PATCH 1/5] Add panic message --- eventsource-client/src/event_parser.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/eventsource-client/src/event_parser.rs b/eventsource-client/src/event_parser.rs index de0e3e6..e21a9d6 100644 --- a/eventsource-client/src/event_parser.rs +++ b/eventsource-client/src/event_parser.rs @@ -281,7 +281,6 @@ impl EventParser { // incomplete lines from previous chunks. fn decode_and_buffer_lines(&mut self, chunk: Bytes) { let mut lines = chunk.split_inclusive(|&b| b == b'\n' || b == b'\r'); - // The first and last elements in this split are special. The spec requires lines to be // terminated. But lines may span chunks, so: // * the last line, if non-empty (i.e. if chunk didn't end with a line terminator), @@ -289,10 +288,8 @@ impl EventParser { // * the first line should be appended to the incomplete line, if any if let Some(incomplete_line) = self.incomplete_line.as_mut() { - let line = lines - .next() - // split always returns at least one item - .unwrap(); + let line = lines.next().expect("Should not be none!"); + // split always returns at least one item trace!( "extending line from previous chunk: {:?}+{:?}", logify(incomplete_line), From febc97d10506d8cdd1ff2425c7fd547cafbc944b Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Thu, 24 Mar 2022 12:58:14 -0700 Subject: [PATCH 2/5] Use cimg/rust --- .circleci/config.yml | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index e71b182..65f7c23 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -2,18 +2,14 @@ version: 2 jobs: build: docker: - - image: circleci/rust:latest - + - image: cimg/rust:1.59.0 steps: - checkout - - run: - name: Install clippy - command: rustup component add clippy - - run: - name: Install rustfmt - command: rustup component add rustfmt - + name: Check Version + command: | + cargo --version + rustc --version - run: name: Check consistent formatting command: cargo fmt && git diff --exit-code From eda88f5a50b6b157e664e7fb2995f60c4e72c506 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Thu, 24 Mar 2022 13:43:15 -0700 Subject: [PATCH 3/5] Fix usage of split_inclusive in event parser logic It was relying on old behavior where Some(empty slice) would be returned if there were no matches. This was updated in more recent rust versions to return None, but CircleCI was using a much older version of rust than I have locally. Updated the usage and CircleCI config to use cimg/rust. --- eventsource-client/src/event_parser.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/eventsource-client/src/event_parser.rs b/eventsource-client/src/event_parser.rs index e21a9d6..6b9f2d2 100644 --- a/eventsource-client/src/event_parser.rs +++ b/eventsource-client/src/event_parser.rs @@ -288,16 +288,14 @@ impl EventParser { // * the first line should be appended to the incomplete line, if any if let Some(incomplete_line) = self.incomplete_line.as_mut() { - let line = lines.next().expect("Should not be none!"); - // split always returns at least one item - trace!( - "extending line from previous chunk: {:?}+{:?}", - logify(incomplete_line), - logify(line) - ); + if let Some(line) = lines.next() { + trace!( + "extending line from previous chunk: {:?}+{:?}", + logify(incomplete_line), + logify(line) + ); - self.last_char_was_cr = false; - if !line.is_empty() { + self.last_char_was_cr = false; // Checking the last character handles lines where the last character is a // terminator, but also where the entire line is a terminator. match line.last().unwrap() { From c746827eacb5ed4d50bd531b6a753e428dbf7484 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Mon, 28 Mar 2022 10:41:04 -0700 Subject: [PATCH 4/5] Add a property-test to exercise parsing logic --- eventsource-client/Cargo.toml | 2 ++ eventsource-client/src/event_parser.rs | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/eventsource-client/Cargo.toml b/eventsource-client/Cargo.toml index 475e210..75c623b 100644 --- a/eventsource-client/Cargo.toml +++ b/eventsource-client/Cargo.toml @@ -27,6 +27,8 @@ maplit = "1.0.1" simplelog = "0.5.3" tokio = { version = "1.2.0", features = ["macros", "rt-multi-thread"] } test-case = "1.2.3" +proptest = "1.0.0" + [features] default = ["rustls"] diff --git a/eventsource-client/src/event_parser.rs b/eventsource-client/src/event_parser.rs index 6b9f2d2..2cbbcb5 100644 --- a/eventsource-client/src/event_parser.rs +++ b/eventsource-client/src/event_parser.rs @@ -289,6 +289,11 @@ impl EventParser { if let Some(incomplete_line) = self.incomplete_line.as_mut() { if let Some(line) = lines.next() { + assert!( + !line.is_empty(), + "split_inclusive should never yield an empty line" + ); + trace!( "extending line from previous chunk: {:?}+{:?}", logify(incomplete_line), @@ -370,6 +375,7 @@ impl EventParser { #[cfg(test)] mod tests { use super::{Error::*, *}; + use proptest::proptest; use test_case::test_case; fn field<'a>(key: &'a str, value: &'a str) -> Result> { @@ -666,4 +672,13 @@ mod tests { std::fs::read(format!("test-data/{}", name)) .unwrap_or_else(|_| panic!("couldn't read {}", name)) } + + proptest! { + #[test] + fn test_decode_and_buffer_lines_does_not_crash(next in "(\r\n|\r|\n)*event: [^\n\r:]*(\r\n|\r|\n)", previous in "(\r\n|\r|\n)*event: [^\n\r:]*(\r\n|\r|\n)") { + let mut parser = EventParser::new(); + parser.incomplete_line = Some(previous.as_bytes().to_vec()); + parser.decode_and_buffer_lines(Bytes::from(next)); + } + } } From 667e494cde08059350a7adb052f78c547a2c9a99 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Mon, 28 Mar 2022 11:16:04 -0700 Subject: [PATCH 5/5] Re-add is_empty() check to line parser --- eventsource-client/src/event_parser.rs | 39 ++++++++++++-------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/eventsource-client/src/event_parser.rs b/eventsource-client/src/event_parser.rs index 2cbbcb5..b3fcea4 100644 --- a/eventsource-client/src/event_parser.rs +++ b/eventsource-client/src/event_parser.rs @@ -289,11 +289,6 @@ impl EventParser { if let Some(incomplete_line) = self.incomplete_line.as_mut() { if let Some(line) = lines.next() { - assert!( - !line.is_empty(), - "split_inclusive should never yield an empty line" - ); - trace!( "extending line from previous chunk: {:?}+{:?}", logify(incomplete_line), @@ -301,22 +296,24 @@ impl EventParser { ); self.last_char_was_cr = false; - // Checking the last character handles lines where the last character is a - // terminator, but also where the entire line is a terminator. - match line.last().unwrap() { - b'\r' => { - incomplete_line.extend_from_slice(&line[..line.len() - 1]); - let il = self.incomplete_line.take(); - self.complete_lines.push_back(il.unwrap()); - self.last_char_was_cr = true; - } - b'\n' => { - incomplete_line.extend_from_slice(&line[..line.len() - 1]); - let il = self.incomplete_line.take(); - self.complete_lines.push_back(il.unwrap()); - } - _ => incomplete_line.extend_from_slice(line), - }; + if !line.is_empty() { + // Checking the last character handles lines where the last character is a + // terminator, but also where the entire line is a terminator. + match line.last().unwrap() { + b'\r' => { + incomplete_line.extend_from_slice(&line[..line.len() - 1]); + let il = self.incomplete_line.take(); + self.complete_lines.push_back(il.unwrap()); + self.last_char_was_cr = true; + } + b'\n' => { + incomplete_line.extend_from_slice(&line[..line.len() - 1]); + let il = self.incomplete_line.take(); + self.complete_lines.push_back(il.unwrap()); + } + _ => incomplete_line.extend_from_slice(line), + }; + } } }