sort: create temporary files private to the owner by sylvestre · Pull Request #14536 · uutils/coreutils

sylvestre · 2026-09-13T16:05:50Z

No description provided.

Copilot

🟡 Changes recommended

Three final findings, including critical and moderate issues, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens sort temporary storage so scratch files are private to the owner on Unix.

Changes:

Enables uucore safe-copy support.
Uses restrictive 0700 directory and 0600 file permissions.
Adds Unix permission tests.

File summaries

File	Summary
`src/uucore/Cargo.toml`	Enables the `safe-copy` feature.
`src/uu/sort/Cargo.toml`	Activates `uucore/safe-copy`.
`src/uu/sort/src/tmp_dir.rs`	Restricts temporary storage and adds tests. Final findings: critical (2 votes) for using `&PathBuf` instead of `&Path`; moderate (3 votes) for unsafe process-wide umask handling; nit (2 votes) for the unversioned GNU behavior claim.

Review details

Suppressed comments (1)

src/uu/sort/src/tmp_dir.rs:35

The non-Unix overload has the same &PathBuf parameter and will trigger the same ptr_arg warning when that configuration is linted. Take &std::path::Path here as well.

fn create_tmp_file(path: &PathBuf) -> std::io::Result<File> {

Files reviewed: 3/3 changed files
Comments generated: 3
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

Unresolved critical permission regression and moderate test-coverage issue must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/uu/sort/src/tmp_dir.rs:159

create_dest_restrictive passes 0o600 to open, so a valid umask such as 0o777 can still create a mode-000 file. The initial descriptor works, but the later File::open calls in merge.rs then fail with EACCES; apply set_permissions/fchmod to the returned descriptor before exposing the path (or make the helper do so).

        let file = uucore::safe_copy::create_dest_restrictive(&path, true);

Files reviewed: 3/3 changed files
Comments generated: 2
Review effort level: Lite

+        // exist at the path yet: a symlink there is hostile, not something to
+        // write through.
+        #[cfg(unix)]
+        let file = uucore::safe_copy::create_dest_restrictive(&path, true);


github-actions · 2026-09-13T17:17:46Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/seq/seq-epipe is now passing!

When the input does not fit in memory, sort spills sorted chunks to files under TMPDIR. The temporary directory was created by tempfile's TempDir (0777 & ~umask) and the chunks by File::create (0666 & ~umask), so under the usual 022 umask they came out 0755 and 0644: every local user could read the input being sorted, in sorted pieces, for as long as the sort ran. GNU sort (9.11) creates its temporaries 0600 whatever the umask. Create the directory 0700, and the chunks through the existing uucore::safe_copy::create_dest_restrictive, which opens 0600 with O_NOFOLLOW and O_CLOEXEC. Nothing should exist at a chunk path yet, so a symlink there is hostile rather than something to write through. The files are restricted as well as the directory so that a directory whose mode is later relaxed does not expose the data. The output-is-input path needs the same care: it took a temporary file from next_file and then overwrote it with fs::copy, which carries the source's permission bits across and put a 0644 output file's mode back on the copy. Write through the descriptor next_file already opened. safe_copy uses rustix::fs but the safe-copy feature did not declare it; it built only because every current user also enables uucore's "fs". Declare it so sort is not the second one relying on that.

Copilot

🟡 Changes recommended

Two moderate correctness issues and one safety-documentation nit remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/uu/sort/src/tmp_dir.rs:159

create_dest_restrictive passes 0o600 to open(2), but that mode is still masked by umask. For example, under umask 0o777 the chunk can be created as 0o000, and the later File::open calls used during merging will fail with EACCES. Set the returned descriptor's permissions to 0o600 before returning/using it, and add a restrictive-umask test.

        let file = uucore::safe_copy::create_dest_restrictive(&path, true);

src/uu/sort/src/tmp_dir.rs:236

Please add a // SAFETY: justification for this separate libc::umask call; the existing comment only documents the unsafe block in set, while this block is another FFI call.

            unsafe { libc::umask(self.0) };

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Lite

+                    let mut source = File::open(&output_path)
+                        .map_err(|error| SortError::OpenTmpFileFailed { error })?;


+        #[cfg(unix)]
+        builder.permissions(Permissions::from_mode(0o700));
+        self.temp_dir = Some(builder.tempdir_in(&self.parent_path).map_err(|_| {
+            SortError::TmpFileCreationFailed {
+                path: self.parent_path.clone(),
+            }
+        })?);


Copilot AI lite review requested due to automatic review settings September 13, 2026 16:05

Copilot started reviewing on behalf of sylvestre September 13, 2026 16:06 View session

Copilot AI reviewed Sep 13, 2026

View reviewed changes

Comment thread src/uu/sort/src/tmp_dir.rs Outdated

Comment thread src/uu/sort/src/tmp_dir.rs Outdated

Comment thread src/uu/sort/src/tmp_dir.rs Outdated

sylvestre force-pushed the sort-tmp-file-permissions branch from 4c1c092 to b07e3f2 Compare September 13, 2026 16:34

Copilot AI review requested due to automatic review settings September 13, 2026 16:34

Copilot started reviewing on behalf of sylvestre September 13, 2026 16:34 View session

Copilot AI reviewed Sep 13, 2026

View reviewed changes

Comment thread src/uu/sort/src/tmp_dir.rs

// exist at the path yet: a symlink there is hostile, not something to

// write through.

#[cfg(unix)]

let file = uucore::safe_copy::create_dest_restrictive(&path, true);

Comment thread src/uu/sort/src/tmp_dir.rs Outdated

sylvestre force-pushed the sort-tmp-file-permissions branch from b07e3f2 to f4c28df Compare September 14, 2026 06:55

Copilot AI review requested due to automatic review settings September 14, 2026 06:55

sylvestre force-pushed the sort-tmp-file-permissions branch from f4c28df to 7c61f67 Compare September 14, 2026 06:59

Copilot started reviewing on behalf of sylvestre September 14, 2026 07:07 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

sort: create temporary files private to the owner - #14536

sort: create temporary files private to the owner#14536
sylvestre wants to merge 1 commit into
uutils:mainfrom
sylvestre:sort-tmp-file-permissions

sylvestre commented Sep 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Sep 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let mut source = File::open(&output_path)
		.map_err(\|error\| SortError::OpenTmpFileFailed { error })?;

Uh oh!

Uh oh!

Conversation

sylvestre commented Sep 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

github-actions Bot commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Sep 13, 2026 •

edited

Loading