From 7276f6d24250a2ad61fc4533ae90303b0483b529 Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Tue, 13 Sep 2022 20:22:34 +0200 Subject: [PATCH 1/3] Fix crash in `refill` when the first line consists of prefix chars The crash is actually in `unfill`. After processing the first line, we would erroneously use the `options.initial_indent` to index into the second line. This lead to a panic when the second line is shorter than the first line. --- src/lib.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 426770d5..1d511fd9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -681,8 +681,8 @@ pub fn unfill(text: &str) -> (String, Options<'_>) { let mut unfilled = String::with_capacity(text.len()); let mut detected_line_ending = None; - for (line, ending) in line_ending::NonEmptyLines(text) { - if unfilled.is_empty() { + for (idx, (line, ending)) in line_ending::NonEmptyLines(text).enumerate() { + if idx == 0 { unfilled.push_str(&line[options.initial_indent.len()..]); } else { unfilled.push(' '); @@ -1850,6 +1850,17 @@ mod tests { assert_eq!(options.subsequent_indent, "> "); } + #[test] + fn unfill_only_prefixes_issue_466() { + // Test that we don't crash if the first line has only prefix + // chars *and* the second line is shorter than the first line. + let (text, options) = unfill("######\nfoo"); + assert_eq!(text, " foo"); + assert_eq!(options.width, 6); + assert_eq!(options.initial_indent, "######"); + assert_eq!(options.subsequent_indent, ""); + } + #[test] fn unfill_whitespace() { assert_eq!(unfill("foo bar").0, "foo bar"); From 4cc3af69bfa0dd6e099ff19fe922de14b2c84282 Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Thu, 15 Sep 2022 22:11:34 +0200 Subject: [PATCH 2/3] Fix crash in `unfill` when string ends with newlines Test that we don't crash on a `\r` following a string of `\n`. The problem was that we removed both kinds of characters in one code path, but not in the other. We now use the same `text` in both places and this fixes the issue. The fix changed the handling of newlines slightly: we now normalize the newlines to a single `\n` or `\r\n` depending on what we detect in the input. The documentation was updated to explicitly say that passing in multiple paragraphs result in unspecified behavior. --- src/lib.rs | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1d511fd9..c252493d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -632,7 +632,9 @@ where /// comments (`'#'` and `'/'`). /// /// The text must come from a single wrapped paragraph. This means -/// that there can be no `"\n\n"` within the text. +/// that there can be no empty lines (`"\n\n"` or `"\r\n\r\n"`) within +/// the text. It is unspecified what happens if `unfill` is called on +/// more than one paragraph of text. /// /// # Examples /// @@ -651,12 +653,10 @@ where /// assert_eq!(options.line_ending, LineEnding::LF); /// ``` pub fn unfill(text: &str) -> (String, Options<'_>) { - let line_ending_pat: &[_] = &['\r', '\n']; - let trimmed = text.trim_end_matches(line_ending_pat); let prefix_chars: &[_] = &[' ', '-', '+', '*', '>', '#', '/']; let mut options = Options::new(0); - for (idx, line) in trimmed.lines().enumerate() { + for (idx, line) in text.lines().enumerate() { options.width = std::cmp::max(options.width, core::display_width(line)); let without_prefix = line.trim_start_matches(prefix_chars); let prefix = &line[..line.len() - without_prefix.len()]; @@ -694,7 +694,13 @@ pub fn unfill(text: &str) -> (String, Options<'_>) { _ => (), } } - unfilled.push_str(&text[trimmed.len()..]); + + // Add back a line ending if `text` ends with the one we detect. + if let Some(line_ending) = detected_line_ending { + if text.ends_with(line_ending.as_str()) { + unfilled.push_str(line_ending.as_str()); + } + } options.line_ending = detected_line_ending.unwrap_or(LineEnding::LF); (unfilled, options) @@ -1772,9 +1778,6 @@ mod tests { assert_eq!(options.line_ending, LineEnding::CRLF); } - /// If mixed new line sequence is encountered, we want to fallback to `\n` - /// 1. it is the default - /// 2. it still matches both `\n` and `\r\n` unlike `\r\n` which will not match `\n` #[test] fn unfill_mixed_new_lines() { let (text, options) = unfill("foo\r\nbar\nbaz"); @@ -1786,14 +1789,14 @@ mod tests { #[test] fn unfill_trailing_newlines() { let (text, options) = unfill("foo\nbar\n\n\n"); - assert_eq!(text, "foo bar\n\n\n"); + assert_eq!(text, "foo bar\n"); assert_eq!(options.width, 3); } #[test] fn unfill_mixed_trailing_newlines() { let (text, options) = unfill("foo\r\nbar\n\r\n\n"); - assert_eq!(text, "foo bar\n\r\n\n"); + assert_eq!(text, "foo bar\n"); assert_eq!(options.width, 3); assert_eq!(options.line_ending, LineEnding::LF); } @@ -1861,6 +1864,19 @@ mod tests { assert_eq!(options.subsequent_indent, ""); } + #[test] + fn unfill_trailing_newlines_issue_466() { + // Test that we don't crash on a '\r' following a string of + // '\n'. The problem was that we removed both kinds of + // characters in one code path, but not in the other. + let (text, options) = unfill("foo\n##\n\n\r"); + // The \n\n changes subsequent_indent to "". + assert_eq!(text, "foo ## \r"); + assert_eq!(options.width, 3); + assert_eq!(options.initial_indent, ""); + assert_eq!(options.subsequent_indent, ""); + } + #[test] fn unfill_whitespace() { assert_eq!(unfill("foo bar").0, "foo bar"); From 0a75ed9578071a79b03c9d9ed4e1cf113ae4edab Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Tue, 13 Sep 2022 23:48:52 +0200 Subject: [PATCH 3/3] Add fuzz tests for `unfill` and `refill` Thanks to @A-Walrus in #466 for the idea. --- .github/workflows/build.yml | 2 ++ fuzz/Cargo.toml | 12 ++++++++++++ fuzz/fuzz_targets/refill.rs | 6 ++++++ fuzz/fuzz_targets/unfill.rs | 6 ++++++ 4 files changed, 26 insertions(+) create mode 100644 fuzz/fuzz_targets/refill.rs create mode 100644 fuzz/fuzz_targets/unfill.rs diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 0a1975d0..c814a0f3 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -120,6 +120,8 @@ jobs: - wrap_first_fit - wrap_optimal_fit - wrap_optimal_fit_usize + - unfill + - refill steps: - name: Checkout repository diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 74c04b45..8f4817f9 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -47,3 +47,15 @@ name = "wrap_optimal_fit_usize" path = "fuzz_targets/wrap_optimal_fit_usize.rs" test = false doc = false + +[[bin]] +name = "refill" +path = "fuzz_targets/refill.rs" +test = false +doc = false + +[[bin]] +name = "unfill" +path = "fuzz_targets/unfill.rs" +test = false +doc = false diff --git a/fuzz/fuzz_targets/refill.rs b/fuzz/fuzz_targets/refill.rs new file mode 100644 index 00000000..16073d0d --- /dev/null +++ b/fuzz/fuzz_targets/refill.rs @@ -0,0 +1,6 @@ +#![no_main] +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|input: (String, usize)| { + let _ = textwrap::refill(&input.0, input.1); +}); diff --git a/fuzz/fuzz_targets/unfill.rs b/fuzz/fuzz_targets/unfill.rs new file mode 100644 index 00000000..f9a1bfb3 --- /dev/null +++ b/fuzz/fuzz_targets/unfill.rs @@ -0,0 +1,6 @@ +#![no_main] +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|input: String| { + let _ = textwrap::unfill(&input); +});