Merge pull request #467 from mgeisler/fix-refill-crashes

Fix `unfill` and `refill` crashes

.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

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

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);
+});

fuzz/fuzz_targets/unfill.rs

@@ -0,0 +1,6 @@
+#![no_main]
+use libfuzzer_sys::fuzz_target;
+
+fuzz_target!(|input: String| {
+    let _ = textwrap::unfill(&input);
+});

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()];
@@ -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(' ');
@@ -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);
     }
@@ -1850,6 +1853,30 @@ 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_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");