Skip to content

dd: remove unnecessary fcntl dependency for stdout #12208

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

Closed
kimjune01 wants to merge 4 commits into from
+103 −16
Closed

dd: remove unnecessary fcntl dependency for stdout #12208

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d5b762
dd: avoid fcntl syscall when using stdout
kimjune01 May 9, 2026
46cec8b
Ensure stdout is never dropped to prevent data loss
kimjune01 May 11, 2026
017c624
fix clippy explicit_auto_deref warnings in FileFromStdout arms
kimjune01 May 12, 2026
7f0b90a
Merge branch 'main' into fix-dd-fcntl-dependency
kimjune01 May 13, 2026
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
38 changes: 25 additions & 13 deletions src/uu/dd/src/dd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use progress::ProgUpdateType;
use progress::{ProgUpdate, ReadStat, StatusLevel, WriteStat, gen_prog_updater};
#[cfg(target_os = "linux")]
use progress::{check_and_reset_sigusr1, install_sigusr1_handler};
use uucore::io::OwnedFileDescriptorOrHandle;
use uucore::translate;

use std::cmp;
Expand All @@ -35,6 +34,7 @@ use std::ffi::OsString;
use std::fs::Metadata;
use std::fs::{File, OpenOptions};
use std::io::{self, Read, Seek, SeekFrom, Write};
use std::mem::ManuallyDrop;
#[cfg(any(target_os = "linux", target_os = "android"))]
use std::os::fd::AsFd;
#[cfg(any(target_os = "linux", target_os = "android"))]
Expand All @@ -45,7 +45,7 @@ use std::os::unix::{
io::{AsRawFd, FromRawFd},
};
#[cfg(windows)]
use std::os::windows::{fs::MetadataExt, io::AsHandle};
use std::os::windows::{fs::MetadataExt, io::{AsHandle, AsRawHandle, FromRawHandle}};
use std::path::Path;
use std::sync::atomic::AtomicU8;
use std::sync::{Arc, atomic::Ordering::Relaxed, mpsc};
Expand Down Expand Up @@ -827,8 +827,12 @@ struct Output<'a> {
impl<'a> Output<'a> {
/// Instantiate this struct with stdout as a destination.
fn new_stdout(settings: &'a Settings) -> UResult<Self> {
let fx = OwnedFileDescriptorOrHandle::from(io::stdout())?;
let mut dst = Dest::Stdout(fx.into_file());
// Use a "borrowed" File to avoid fcntl syscall from try_clone_to_owned
#[cfg(unix)]
let file = ManuallyDrop::new(unsafe { File::from_raw_fd(io::stdout().as_raw_fd()) });
#[cfg(windows)]
let file = ManuallyDrop::new(unsafe { File::from_raw_handle(io::stdout().as_raw_handle()) });
let mut dst = Dest::Stdout(ManuallyDrop::into_inner(file));
dst.seek(settings.seek, settings.obs)
.map_err_context(|| translate!("dd-error-write-error"))?;
Ok(Self { dst, settings })
Expand Down Expand Up @@ -888,16 +892,24 @@ impl<'a> Output<'a> {
/// already opened by the system (stdout) and has a state
/// (current position) that shall be used.
fn new_file_from_stdout(settings: &'a Settings) -> UResult<Self> {
let fx = OwnedFileDescriptorOrHandle::from(io::stdout())?;
#[cfg(any(target_os = "linux", target_os = "android"))]
if let Some(libc_flags) = make_linux_oflags(&settings.oflags) {
nix::fcntl::fcntl(
fx.as_raw().as_fd(),
FcntlArg::F_SETFL(OFlag::from_bits_retain(libc_flags)),
)?;
}
// Use a "borrowed" File to avoid fcntl syscall from try_clone_to_owned
#[cfg(unix)]
let file = {
let stdout = io::stdout();
let raw_fd = stdout.as_raw_fd();
#[cfg(any(target_os = "linux", target_os = "android"))]
if let Some(libc_flags) = make_linux_oflags(&settings.oflags) {
nix::fcntl::fcntl(
raw_fd,
FcntlArg::F_SETFL(OFlag::from_bits_retain(libc_flags)),
)?;
}
ManuallyDrop::new(unsafe { File::from_raw_fd(raw_fd) })
};
#[cfg(windows)]
let file = ManuallyDrop::new(unsafe { File::from_raw_handle(io::stdout().as_raw_handle()) });

Self::prepare_file(fx.into_file(), settings)
Self::prepare_file(ManuallyDrop::into_inner(file), settings)
Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze May 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is potentially unsound, we must guarantee stdout is never dropped.

Copy link
Copy Markdown
Author

@kimjune01 kimjune01 May 11, 2026

Choose a reason for hiding this comment

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

You're absolutely correct. The previous version created a File from stdout's raw fd without duplicating it, which meant when the File was dropped, it would close stdout's fd and potentially lose buffered data.

I've fixed this by:

  1. Changing Dest::Stdout to hold ManuallyDrop<File> instead of File
  2. Adding a new Dest::FileFromStdout variant for when stdout is redirected to a seekable file
  3. Both variants now guarantee stdout's file descriptor is never closed when dropped

This preserves the optimization of avoiding the fcntl syscall while maintaining soundness. All tests pass.

Thanks for catching this!

}

/// Instantiate this struct with the given named pipe as a destination.
Expand Down
Loading