Skip to content

fix(util): honor the requested random output path - #877

Merged
kvinwang merged 1 commit into
masterfrom
codex/fix-util-random-output-path
Aug 4, 2026
Merged

fix(util): honor the requested random output path#877
kvinwang merged 1 commit into
masterfrom
codex/fix-util-random-output-path

Conversation

@kvinwang

@kvinwang kvinwang commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Problem

dstack-util rand parsed -o/--output and then ignored it, always writing to stdout. The command reported success while the requested file was never created.

Fix

Write the file through safe_write_with_mode, which this crate already depends on (dstack-util/Cargo.toml:65, and system_setup.rs in the same crate already uses safe_write), rather than hand-rolling the write:

safe_write::safe_write_with_mode(&output, &data, 0o600)
    .with_context(|| format!("Failed to write random output {output}"))?;

Why not a hand-rolled create_new + write_all + sync_all

That was the first version of this PR. Three problems, measured rather than assumed:

1. after a failed write: file exists = true, contents = "PARTIAL", size = 7
2. retry:                FAILED -> File exists (os error 17) (AlreadyExists)
   file still contains:  "PARTIAL"
3. safe_write retry:     ok, contents = "REAL-...-SECRET", mode = 600
  • A failed write leaves a truncated secret in place. A 7-byte "random" file at 0600 looks exactly like a successful result. Nothing downstream can distinguish it from a real one, and for key material a short file is a catastrophically weak secret.
  • The retry then fails, because create_new refuses the file the failed run left behind. A transient ENOSPC wedges the command permanently, with the partial secret still on disk.
  • sync_all alone is not durable. It flushes the file's contents but not the directory entry, so a crash can leave the data on disk with nothing pointing at it — the exact failure safe-write exists to prevent.

safe_write_with_mode creates the file 0600 before any content is written, publishes it in a single rename, fsyncs both the file and its parent directory, and removes the temporary file on every error path.

It also drops the fs_err::os::unix::fs::OpenOptionsExt import entirely, so the second commit of the original branch is no longer needed. The two commits have been squashed into one.

Behaviour note

Re-running now replaces an existing file instead of failing, matching openssl rand -out. The previous create_new was introduced by this PR rather than inherited, so nothing depended on it. If a no-clobber mode is wanted it should be an explicit flag, not the implicit default.

Verification

Four tests, all confirmed to fail against the previous behaviour rather than merely passing:

test before after
rand_writes_to_the_requested_output_path FAIL pass
rand_output_is_owner_only FAIL pass
rand_hex_output_is_twice_as_long FAIL pass
rand_replaces_an_existing_output FAIL pass

62 tests pass in dstack-util, cargo fmt --check is clean and clippy reports 0 warnings. Rebased onto current master.

Related

The safe-write behind this call is pinned at 0.1.3, which has known correctness bugs of its own. #989 upgrades the workspace to 0.2.0 and switches dstack-cli-core::fsutil onto it. This PR is correct either way — the call site does not change — but the guarantees described above only fully hold once #989 lands.

Copilot AI review requested due to automatic review settings July 31, 2026 02:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kvinwang
kvinwang force-pushed the codex/fix-util-random-output-path branch from 15beff3 to 3796c60 Compare August 4, 2026 04:24
`dstack-util rand` parsed `-o/--output` and then ignored it, always writing to
stdout. The command reported success while the requested file was never
created.

Write the file through `safe_write_with_mode`, which the workspace already
depends on, rather than hand-rolling the write:

- The output is key material, so it is created 0600 and never exists under
  wider permissions.
- The replacement is a single rename, and both the file and its directory are
  fsynced. A truncated random file is a silently weak secret — it looks exactly
  like a successful result, and nothing downstream can tell the difference.
- If any step fails, nothing is left behind. A hand-rolled `create_new` + write
  leaves a short file that the next run then refuses to replace, so a transient
  ENOSPC would wedge the command with a partial secret in place.

Re-running replaces the file rather than failing on an existing one, matching
`openssl rand -out`. If a no-clobber mode is wanted later it should be an
explicit flag rather than the implicit default.

Four tests, all confirmed to fail against the previous behaviour: the file is
created at the requested path with no temporary left behind, it is 0600, `-x`
doubles the length and emits hex, and a re-run replaces it.
@kvinwang
kvinwang force-pushed the codex/fix-util-random-output-path branch from 3796c60 to 1d46575 Compare August 4, 2026 04:26
@kvinwang
kvinwang enabled auto-merge August 4, 2026 04:27
@kvinwang
kvinwang merged commit 3289c90 into master Aug 4, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants