Repository: mgeisler/textwrap (language: rust)

Issue:
Bugs with refill
1. Running refill with certain inputs will crash.
2. Running refill, once, then again on the results, with the same options, doesn't always produce the same results.

Wrote some fuzz targets to test:
1. 
```rust
#![no_main]
use libfuzzer_sys::fuzz_target;

fuzz_target!(|input: (String, usize)| {
    let _ = textwrap::refill(&input.0, input.1);
});

```
2.
```rust
#![no_main]
use libfuzzer_sys::fuzz_target;
use textwrap::refill;

fuzz_target!(|input: String| {
    let len = 20;
    let refilled = refill(&input, len);
    let refilled_again = refill(&refilled, len);
    assert_eq!(refilled, refilled_again);
});
```

Downstream issue in Helix: helix-editor/helix#3734


Discussion / hints:
Hi @A-Walrus, thanks a lot for the report. It's super useful with the fuzz test... I'll run them and see what I find.

Fix patch:
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);
+});
diff --git a/src/lib.rs b/src/lib.rs
index 426770d5..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()];
@@ -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");


Test patch: