Conversation
A huge --tabsize was accepted and then used to size the tab expansion buffer, so the first tab in the input made diff overflow or abort on allocation failure. Reject values above isize::MAX - 3 with "invalid tabsize" and exit status 2, as GNU diff does.
| use regex::Regex; | ||
|
|
||
| /// Largest value accepted for `--tabsize`, matching GNU diff. | ||
| const MAX_TABSIZE: usize = isize::MAX as usize - 3; |
There was a problem hiding this comment.
how did you get - 3?
it's not in the manual, so where does this value come from?
There was a problem hiding this comment.
I found it by testing GNU diff 3.12, GNU accepts the one ending in …804 and rejects the one ending in …805. happy to change the exact limit if you want
| num | ||
| } | ||
| Err(_) => return Err(format!("invalid tabsize «{tabsize_str}»")), | ||
| Ok(num) if num != 0 && num <= MAX_TABSIZE => num, |
There was a problem hiding this comment.
this doesn't fix the crash, it just moves it
-t --tabsize=9223372036854775804 with a tab in the input still aborts in do_expand_tabs (with_capacity + ntabs * (tabsize - 1) overflow)
could you please fix do_expand_tabs instead (no huge preallocation, write the spaces as a stream)?
There was a problem hiding this comment.
You're right, it did just move the crash. I've changed do_expand_tabs the way you suggested: it no longer reserves a big block of memory up front, and it writes the spaces in small chunks. One thing it doesn't fix yet is that diff still holds its whole output in memory, so a big enough value will still run out of memory at some point. Do you want me to stream the whole output in this PR, or leave that for a separate follow-up?
| if !option.is_empty() { | ||
| cmd.arg(option); | ||
| } | ||
| cmd.arg("-t") |
There was a problem hiding this comment.
No, it isn't, so I've removed it. The value gets rejected before any input is read, and GNU rejects it without -t as well.
| file1.write_all("\tx\n".as_bytes())?; | ||
| let mut file2 = NamedTempFile::new()?; | ||
| file2.write_all("y\n".as_bytes())?; | ||
| for option in ["", "-u", "-c"] { |
There was a problem hiding this comment.
the value is rejected while parsing, so looping over the formats doesn't test anything more, no?
please add a test with a value below the limit and a tab in the input, that's the one that crashes
There was a problem hiding this comment.
I agree, the loop didn't add anything, so I've removed it. In its place I added a test that calls do_expand_tabs directly with a tab and a huge value just below the limit. It finishes instantly without crashing. A full end-to-end test would need the output to stream first, so that depends on what you decide about my question in the other thread.
2 participants
Fixed an issue where passing an extremely large value to --tabsize could cause diff to crash.
Added an upper limit matching GNU diff.
Values above the limit now return a normal invalid tabsize error.
Added tests for the boundary values and a large --tabsize case.
This keeps the behavior consistent with GNU diff and prevents the crash.
Huge values below the limit (like 2^62) are still accepted, same as GNU, and can still run out of memory. Handling that would need the diff output to be fallible. Let me know if you want that too.
Closes #286