Skip to content

tr: fix high memory use, possible heap exhaustion #8435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sylvestre merged 2 commits into from
Aug 6, 2025
Merged

tr: fix high memory use, possible heap exhaustion #8435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef40c47
tr: fix high memory use, possible heap exhaustion
julian-klode Aug 5, 2025
c93b9ed
tr: use an unbuffered stdout
julian-klode Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/uu/tr/src/operation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,15 +672,17 @@ where
R: BufRead,
W: Write,
{
let mut buf = Vec::new();
let mut buf = [0; 8192];
let mut output_buf = Vec::new();

while let Ok(length) = input.read_until(b'\n', &mut buf) {
while let Ok(length) = input.read(&mut buf[..]) {
if length == 0 {
break; // EOF reached
}

let filtered = buf.iter().filter_map(|&c| translator.translate(c));
let filtered = buf[..length]
.iter()
.filter_map(|&c| translator.translate(c));
output_buf.extend(filtered);
Copy link
Collaborator

@drinkcat drinkcat Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe for a follow-up, but if you want to do benchmarking (and fine-tuning) here, I wonder if we can just directly write_all filtered, instead of collecting the data in output_buf first?

(but then... you might want to restore a buffered writer on stdout)

Copy link
Contributor Author

@julian-klode julian-klode Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should deal with performance separately, this is a surprisingly big issue:

$ hyperfine 'gnutr a b <  zeroes'
Benchmark 1: gnutr a b <  zeroes
  Time (mean ± σ):     558.3 ms ±  80.6 ms    [User: 356.6 ms, System: 201.5 ms]
  Range (min … max):   528.3 ms … 787.5 ms    10 runs

$ hyperfine './target/release/tr a b <  zeroes'
Benchmark 1: ./target/release/tr a b <  zeroes
 ⠴ Current estimate: 5.681 s

(main is at 6.8 ish)

Copy link
Contributor

@sylvestre sylvestre Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should run hyperfine this way:
hyperfine 'gnutr a b < zeroes' './target/release/tr a b < zeroes'

you get much better output

Copy link
Collaborator

@drinkcat drinkcat Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. Yes, sounds good, would be great if you can file an issue for the performance issue.

(I didn't look at our code for tr, but we use some vector instructions in wc to find newline chars that may also be helpful here.)

Copy link
Contributor Author

@julian-klode julian-klode Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance Bug in #8439


#[cfg(not(target_os = "windows"))]
Expand All @@ -698,7 +700,6 @@ where
}
}

buf.clear();
output_buf.clear();
}

Expand Down
18 changes: 9 additions & 9 deletions src/uu/tr/src/tr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use operation::{
translate_input,
};
use std::ffi::OsString;
use std::io::{BufWriter, Write, stdin, stdout};
use std::io::{Write, stdin, stdout};
use uucore::display::Quotable;
use uucore::error::{FromIo, UResult, USimpleError, UUsageError};
use uucore::fs::is_stdin_directory;
Expand Down Expand Up @@ -107,7 +107,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {

let stdin = stdin();
let mut locked_stdin = stdin.lock();
let mut buffered_stdout = BufWriter::new(stdout().lock());
let mut locked_stdout = stdout().lock();

// According to the man page: translating only happens if deleting or if a second set is given
let translating = !delete_flag && sets.len() > 1;
Expand All @@ -131,34 +131,34 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
let delete_op = DeleteOperation::new(set1);
let squeeze_op = SqueezeOperation::new(set2);
let op = delete_op.chain(squeeze_op);
translate_input(&mut locked_stdin, &mut buffered_stdout, op)?;
translate_input(&mut locked_stdin, &mut locked_stdout, op)?;
} else {
let op = DeleteOperation::new(set1);
translate_input(&mut locked_stdin, &mut buffered_stdout, op)?;
translate_input(&mut locked_stdin, &mut locked_stdout, op)?;
}
} else if squeeze_flag {
if sets_len == 1 {
let op = SqueezeOperation::new(set1);
translate_input(&mut locked_stdin, &mut buffered_stdout, op)?;
translate_input(&mut locked_stdin, &mut locked_stdout, op)?;
} else {
let translate_op = TranslateOperation::new(set1, set2.clone())?;
let squeeze_op = SqueezeOperation::new(set2);
let op = translate_op.chain(squeeze_op);
translate_input(&mut locked_stdin, &mut buffered_stdout, op)?;
translate_input(&mut locked_stdin, &mut locked_stdout, op)?;
}
} else {
let op = TranslateOperation::new(set1, set2)?;
translate_input(&mut locked_stdin, &mut buffered_stdout, op)?;
translate_input(&mut locked_stdin, &mut locked_stdout, op)?;
}

#[cfg(not(target_os = "windows"))]
buffered_stdout
locked_stdout
.flush()
.map_err_context(|| translate!("tr-error-write-error"))?;

// SIGPIPE is not available on Windows.
#[cfg(target_os = "windows")]
match buffered_stdout.flush() {
match locked_stdout.flush() {
Ok(()) => {}
Err(err) if err.kind() == std::io::ErrorKind::BrokenPipe => std::process::exit(13),
Err(err) => return Err(err.map_err_context(|| translate!("tr-error-write-error"))),
Expand Down
Loading