From fe188b04a7e908b676f09cf7c4f59a44a775fb9b Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 18 Aug 2026 23:47:07 +0200 Subject: [PATCH] sort: reject --batch-size arguments above the fd soft limit sort only validated --batch-size against the file descriptor rlimit when the argument did not even fit in a usize. A value such as 20 with a soft limit of 19 was silently accepted, while GNU sort exits with status 2 and reports the maximum usable value. Compare the parsed value with the soft limit minus the three descriptors that are always taken by stdin, stdout and stderr, and reuse the existing "too large"/"maximum --batch-size argument with current rlimit" messages for both the overflow and the out-of-range cases. PR #11961 (deduplicating file descriptors during merge) targets a different part of the problem and does not touch this validation. Should make test tests/sort/sort-merge-fdlimit.sh pass https://github.com/uutils/coreutils/pull/11961 https://github.com/uutils/coreutils/issues/13841 --- src/uu/sort/src/sort.rs | 95 ++++++++++++++++++++------------------ tests/by-util/test_sort.rs | 60 ++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 44 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 6639b744d5..ba8c1e5bce 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -1520,6 +1520,16 @@ pub(crate) fn fd_soft_limit() -> Option { None } +/// The largest `--batch-size` argument that can be honoured with the current file +/// descriptor soft limit, or `None` if that limit is unknown. +/// +/// Three descriptors are always in use (stdin, stdout and stderr) and are therefore +/// not available for merge inputs. +fn max_merge_batch_size() -> Option { + const RESERVED_STDIO: usize = 3; + fd_soft_limit().map(|limit| limit.saturating_sub(RESERVED_STDIO)) +} + #[cfg(unix)] pub(crate) fn current_open_fd_count() -> Option { fn count_dir(path: &str) -> Option { @@ -2259,54 +2269,51 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .map(String::from); if let Some(n_merge) = matches.get_one::(options::BATCH_SIZE) { - match n_merge.parse::() { - Ok(parsed_value) => { - if parsed_value < 2 { - show_error!( - "{}", - translate!("sort-invalid-batch-size-arg", "arg" => n_merge) - ); - return Err(UUsageError::new( - 2, - translate!("sort-minimum-batch-size-two"), - )); - } - settings.merge_batch_size = parsed_value; + // `None` means the value does not even fit in a `usize`, which is always too large. + let parsed_value = match n_merge.parse::() { + Ok(parsed_value) => Some(parsed_value), + Err(e) if *e.kind() == IntErrorKind::PosOverflow => None, + Err(_) => { + return Err(UUsageError::new( + 2, + translate!("sort-invalid-batch-size-arg", "arg" => n_merge), + )); } - Err(e) => { - let error_message = if *e.kind() == IntErrorKind::PosOverflow { - let batch_too_large = translate!( - "sort-batch-size-too-large", - "arg" => n_merge.quote() - ); + }; - #[cfg(target_os = "linux")] - { - show_error!("{batch_too_large}"); - - translate!( - "sort-maximum-batch-size-rlimit", - "rlimit" => { - fd_soft_limit().ok_or_else(|| { - UUsageError::new(2, translate!("sort-failed-fetch-rlimit")) - })? - } - ) - } - #[cfg(not(target_os = "linux"))] - { - batch_too_large - } - } else { - translate!( - "sort-invalid-batch-size-arg", - "arg" => n_merge, - ) - }; + if parsed_value.is_some_and(|value| value < 2) { + show_error!( + "{}", + translate!("sort-invalid-batch-size-arg", "arg" => n_merge) + ); + return Err(UUsageError::new( + 2, + translate!("sort-minimum-batch-size-two"), + )); + } - return Err(UUsageError::new(2, error_message)); - } + let max_batch_size = max_merge_batch_size(); + let too_large = match (parsed_value, max_batch_size) { + (None, _) => true, + (Some(value), Some(max)) => value > max, + (Some(_), None) => false, + }; + if too_large { + let batch_too_large = translate!( + "sort-batch-size-too-large", + "arg" => n_merge.quote() + ); + let error_message = match max_batch_size { + Some(max) => { + show_error!("{batch_too_large}"); + translate!("sort-maximum-batch-size-rlimit", "rlimit" => max) + } + None => batch_too_large, + }; + return Err(UUsageError::new(2, error_message)); } + + settings.merge_batch_size = parsed_value.unwrap_or(usize::MAX); } settings.line_ending = LineEnding::from_zero_flag(matches.get_flag(options::ZERO_TERMINATED)); diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index f12850f28e..49608b7a40 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -1549,6 +1549,66 @@ fn test_merge_batch_size_with_limit() { .stdout_only_fixture("merge_ints_interleaved.expected"); } +#[test] +// TODO(#7542): Re-enable on Android once we figure out why setting limit is broken. +#[cfg(target_os = "linux")] +fn test_batch_size_above_fd_limit_is_rejected() { + use rlimit::Resource; + // Only stdin, stdout and stderr are unavailable for merge inputs, so the + // largest acceptable --batch-size is the soft limit minus 3, here 27 - 3. + let limit_fd = 27; + let (at, mut ucmd) = at_and_ucmd!(); + at.write("gamma.txt", "delta\nalpha\n"); + ucmd.limit(Resource::NOFILE, limit_fd, limit_fd) + .arg("--batch-size=31") + .arg("gamma.txt") + .fails_with_code(2) + .stderr_contains("--batch-size argument '31' too large") + // 24 is forced by the limit above: 27 - 3 reserved descriptors. + .stderr_contains("maximum --batch-size argument with current rlimit is 24"); +} + +#[test] +#[cfg(target_os = "linux")] +fn test_batch_size_at_fd_limit_is_accepted() { + use rlimit::Resource; + let limit_fd = 27; + let (at, mut ucmd) = at_and_ucmd!(); + at.write("gamma.txt", "delta\nalpha\n"); + // 24 is the largest value the limit above allows, and sorting must still happen. + ucmd.limit(Resource::NOFILE, limit_fd, limit_fd) + .arg("--batch-size=24") + .arg("gamma.txt") + .succeeds() + .stdout_only("alpha\ndelta\n"); +} + +#[test] +#[cfg(target_os = "linux")] +fn test_merge_more_files_than_fd_limit() { + use rlimit::Resource; + let (at, mut ucmd) = at_and_ucmd!(); + // 40 single-line files cannot all be open at once with a soft limit of 24, + // so sort has to merge them in several batches through temporary files. + let count = 40; + let mut names = Vec::new(); + for i in 0..count { + let name = format!("fdlimit_{i:02}.txt"); + at.write(&name, &format!("{i:02}\n")); + names.push(name); + } + let mut expected = String::new(); + for i in 0..count { + writeln!(expected, "{i:02}").unwrap(); + } + let limit_fd = 24; + ucmd.limit(Resource::NOFILE, limit_fd, limit_fd) + .arg("-m") + .args(&names) + .succeeds() + .stdout_only(expected); +} + #[test] fn test_sigpipe_panic() { let mut cmd = new_ucmd!();