Skip to content

cp: fix device file creation with -r flag #8693

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

Open
AnarchistHoneybun wants to merge 5 commits into
base: main
Choose a base branch
from
Open

cp: fix device file creation with -r flag #8693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66d4d0d
cp: fix device file creation with -r flag
AnarchistHoneybun Sep 21, 2025
54ccdd8
tests/cp: add regression tests for device file handling
AnarchistHoneybun Sep 22, 2025
4c183ea
Merge branch 'uutils:main' into bug/cp-device-file
AnarchistHoneybun Sep 22, 2025
8d3efa4
Merge branch 'uutils:main' into bug/cp-device-file
AnarchistHoneybun Sep 23, 2025
17710fd
Merge branch 'uutils:main' into bug/cp-device-file
AnarchistHoneybun Sep 27, 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
2 changes: 2 additions & 0 deletions src/uu/cp/locales/en-US.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ cp-error-selinux-set-context = failed to set the security context of { $path }:
cp-error-selinux-get-context = failed to get security context of { $path }
cp-error-selinux-error = SELinux error: { $error }
cp-error-cannot-create-fifo = cannot create fifo { $path }: File exists
cp-error-cannot-create-char-device = cannot create character device { $path }
cp-error-cannot-create-block-device = cannot create block device { $path }
cp-error-invalid-attribute = invalid attribute { $value }
cp-error-failed-to-create-whole-tree = failed to create whole tree
cp-error-failed-to-create-directory = Failed to create directory: { $error }
Expand Down
2 changes: 2 additions & 0 deletions src/uu/cp/locales/fr-FR.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ cp-error-selinux-set-context = échec de la définition du contexte de sécurit
cp-error-selinux-get-context = échec de l'obtention du contexte de sécurité de { $path }
cp-error-selinux-error = Erreur SELinux : { $error }
cp-error-cannot-create-fifo = impossible de créer le fifo { $path } : Le fichier existe
cp-error-cannot-create-char-device = impossible de créer le périphérique caractère { $path }
cp-error-cannot-create-block-device = impossible de créer le périphérique bloc { $path }
cp-error-invalid-attribute = attribut invalide { $value }
cp-error-failed-to-create-whole-tree = échec de la création de l'arborescence complète
cp-error-failed-to-create-directory = Échec de la création du répertoire : { $error }
Expand Down
80 changes: 77 additions & 3 deletions src/uu/cp/src/cp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::ffi::OsString;
use std::fmt::Display;
use std::fs::{self, Metadata, OpenOptions, Permissions};
#[cfg(unix)]
use std::os::unix::fs::{FileTypeExt, PermissionsExt};
use std::os::unix::fs::{FileTypeExt, MetadataExt, PermissionsExt};
#[cfg(unix)]
use std::os::unix::net::UnixListener;
use std::path::{Path, PathBuf, StripPrefixError};
Expand All @@ -27,13 +27,13 @@ use thiserror::Error;
use platform::copy_on_write;
use uucore::display::Quotable;
use uucore::error::{UError, UResult, UUsageError, set_exit_code};
#[cfg(unix)]
use uucore::fs::make_fifo;
use uucore::fs::{
FileInformation, MissingHandling, ResolveMode, are_hardlinks_to_same_file, canonicalize,
get_filename, is_symlink_loop, normalize_path, path_ends_with_terminator,
paths_refer_to_same_file,
};
#[cfg(unix)]
use uucore::fs::{make_block_device, make_char_device, make_fifo};
use uucore::{backup_control, update_control};
// These are exposed for projects (e.g. nushell) that want to create an `Options` value, which
// requires these enum.
Expand Down Expand Up @@ -2082,6 +2082,8 @@ fn handle_copy_mode(
source_in_command_line: bool,
source_is_fifo: bool,
source_is_socket: bool,
source_is_char_device: bool,
source_is_block_device: bool,
#[cfg(unix)] source_is_stream: bool,
) -> CopyResult<PerformedAction> {
let source_is_symlink = source_metadata.is_symlink();
Expand Down Expand Up @@ -2122,6 +2124,8 @@ fn handle_copy_mode(
source_is_symlink,
source_is_fifo,
source_is_socket,
source_is_char_device,
source_is_block_device,
symlinked_files,
#[cfg(unix)]
source_is_stream,
Expand All @@ -2145,6 +2149,8 @@ fn handle_copy_mode(
source_is_symlink,
source_is_fifo,
source_is_socket,
source_is_char_device,
source_is_block_device,
symlinked_files,
#[cfg(unix)]
source_is_stream,
Expand Down Expand Up @@ -2181,6 +2187,8 @@ fn handle_copy_mode(
source_is_symlink,
source_is_fifo,
source_is_socket,
source_is_char_device,
source_is_block_device,
symlinked_files,
#[cfg(unix)]
source_is_stream,
Expand All @@ -2196,6 +2204,8 @@ fn handle_copy_mode(
source_is_symlink,
source_is_fifo,
source_is_socket,
source_is_char_device,
source_is_block_device,
symlinked_files,
#[cfg(unix)]
source_is_stream,
Expand Down Expand Up @@ -2424,10 +2434,18 @@ fn copy_file(
let source_is_fifo = source_metadata.file_type().is_fifo();
#[cfg(unix)]
let source_is_socket = source_metadata.file_type().is_socket();
#[cfg(unix)]
let source_is_char_device = source_metadata.file_type().is_char_device();
#[cfg(unix)]
let source_is_block_device = source_metadata.file_type().is_block_device();
#[cfg(not(unix))]
let source_is_fifo = false;
#[cfg(not(unix))]
let source_is_socket = false;
#[cfg(not(unix))]
let source_is_char_device = false;
#[cfg(not(unix))]
let source_is_block_device = false;

let source_is_stream = is_stream(&source_metadata);

Expand All @@ -2441,6 +2459,8 @@ fn copy_file(
source_in_command_line,
source_is_fifo,
source_is_socket,
source_is_char_device,
source_is_block_device,
#[cfg(unix)]
source_is_stream,
)?;
Expand Down Expand Up @@ -2568,6 +2588,8 @@ fn copy_helper(
source_is_symlink: bool,
source_is_fifo: bool,
source_is_socket: bool,
source_is_char_device: bool,
Copy link
Contributor

@sylvestre sylvestre Sep 21, 2025

Choose a reason for hiding this comment

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

it is starting to be too much, can we refactor this ?

Copy link
Contributor Author

@AnarchistHoneybun AnarchistHoneybun Sep 21, 2025

Choose a reason for hiding this comment

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

could you elaborate a little?

Copy link
Contributor

@sylvestre sylvestre Sep 21, 2025

Choose a reason for hiding this comment

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

the function has too many parameters, we should have less

Copy link
Contributor Author

@AnarchistHoneybun AnarchistHoneybun Sep 21, 2025

Choose a reason for hiding this comment

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

will look into it

Copy link
Contributor Author

@AnarchistHoneybun AnarchistHoneybun Sep 22, 2025

Choose a reason for hiding this comment

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

the function has too many parameters, we should have less

Would something like this be acceptable:

#[derive(Debug, Clone)]
struct SourceFileType {
    is_symlink: bool,
    is_fifo: bool,
    is_socket: bool,
    is_char_device: bool,
    is_block_device: bool,
    #[cfg(unix)]
    is_stream: bool,
}

impl SourceFileType {
    fn from_metadata(metadata: &Metadata) -> Self {
        #[cfg(unix)]
        {
            Self {
                is_symlink: metadata.is_symlink(),
                is_fifo: metadata.file_type().is_fifo(),
                is_socket: metadata.file_type().is_socket(),
                is_char_device: metadata.file_type().is_char_device(),
                is_block_device: metadata.file_type().is_block_device(),
                is_stream: is_stream(metadata),
            }
        }
        #[cfg(not(unix))]
        {
            Self {
                is_symlink: metadata.is_symlink(),
                is_fifo: false,
                is_socket: false,
                is_char_device: false,
                is_block_device: false,
            }
        }
    }
    
    fn is_special_file(&self) -> bool {
        self.is_fifo || self.is_socket || self.is_char_device || self.is_block_device
    }
}
...

fn handle_copy_mode(
    source: &Path,
    dest: &Path,
    options: &Options,
    context: &str,
    source_metadata: &Metadata,
    symlinked_files: &mut HashSet<FileInformation>,
    source_in_command_line: bool,
    source_file_type: &SourceFileType,
) -> CopyResult<PerformedAction>

Copy link

@bdrung bdrung Sep 22, 2025

Choose a reason for hiding this comment

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

Why not use an enum here? All those file types (symlink, fifo, socket, char, block) are distinct.

Copy link
Contributor Author

@AnarchistHoneybun AnarchistHoneybun Sep 22, 2025

Choose a reason for hiding this comment

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

hmm, what about stream? (apologise if these questions are stupid, I'm less familiar with the linux environment than I am with Rust, I thought of an enum at first but then stopped short imagining well if the first person to do it didn't make it an enum, maybe it's possible they can happen simultaneously)

Copy link

@bdrung bdrung Sep 22, 2025

Choose a reason for hiding this comment

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

I checked the is_stream functions and it says:

file_type.is_fifo() || file_type.is_char_device() || file_type.is_block_device()

So the is_stream struct element could be dropped and converted into a check for matching the enum.

source_is_block_device: bool,
symlinked_files: &mut HashSet<FileInformation>,
#[cfg(unix)] source_is_stream: bool,
) -> CopyResult<()> {
Expand All @@ -2586,6 +2608,12 @@ fn copy_helper(
} else if source_is_fifo && options.recursive && !options.copy_contents {
#[cfg(unix)]
copy_fifo(dest, options.overwrite, options.debug)?;
} else if source_is_char_device && options.recursive && !options.copy_contents {
#[cfg(unix)]
copy_char_device(source, dest, options.overwrite, options.debug)?;
} else if source_is_block_device && options.recursive && !options.copy_contents {
#[cfg(unix)]
copy_block_device(source, dest, options.overwrite, options.debug)?;
} else if source_is_symlink {
copy_link(source, dest, symlinked_files, options)?;
} else {
Expand Down Expand Up @@ -2620,6 +2648,52 @@ fn copy_fifo(dest: &Path, overwrite: OverwriteMode, debug: bool) -> CopyResult<(
.map_err(|_| translate!("cp-error-cannot-create-fifo", "path" => dest.quote()).into())
}

// "Copies" a character device by creating a new one with the same major/minor numbers.
#[cfg(unix)]
fn copy_char_device(
source: &Path,
dest: &Path,
overwrite: OverwriteMode,
debug: bool,
) -> CopyResult<()> {
if dest.exists() {
overwrite.verify(dest, debug)?;
fs::remove_file(dest)?;
}

let source_metadata = fs::metadata(source)?;
let device_id = source_metadata.rdev();
let major = ((device_id >> 8) & 0xff) as u32;
let minor = (device_id & 0xff) as u32;

make_char_device(dest, major, minor).map_err(|_| {
translate!("cp-error-cannot-create-char-device", "path" => dest.quote()).into()
})
}

// "Copies" a block device by creating a new one with the same major/minor numbers.
#[cfg(unix)]
fn copy_block_device(
source: &Path,
dest: &Path,
overwrite: OverwriteMode,
debug: bool,
) -> CopyResult<()> {
if dest.exists() {
overwrite.verify(dest, debug)?;
fs::remove_file(dest)?;
}

let source_metadata = fs::metadata(source)?;
let device_id = source_metadata.rdev();
let major = ((device_id >> 8) & 0xff) as u32;
let minor = (device_id & 0xff) as u32;

make_block_device(dest, major, minor).map_err(|_| {
translate!("cp-error-cannot-create-block-device", "path" => dest.quote()).into()
})
}

#[cfg(unix)]
fn copy_socket(dest: &Path, overwrite: OverwriteMode, debug: bool) -> CopyResult<()> {
if dest.exists() {
Expand Down
38 changes: 37 additions & 1 deletion src/uucore/src/lib/features/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use libc::{
S_IFBLK, S_IFCHR, S_IFDIR, S_IFIFO, S_IFLNK, S_IFMT, S_IFREG, S_IFSOCK, S_IRGRP, S_IROTH,
S_IRUSR, S_ISGID, S_ISUID, S_ISVTX, S_IWGRP, S_IWOTH, S_IWUSR, S_IXGRP, S_IXOTH, S_IXUSR,
mkfifo, mode_t,
makedev, mkfifo, mknod, mode_t,
};
use std::collections::HashSet;
use std::collections::VecDeque;
Expand Down Expand Up @@ -837,6 +837,42 @@ pub fn make_fifo(path: &Path) -> std::io::Result<()> {
}
}

/// Create a character device file.
///
/// # Arguments
/// * `path` - The path where the device file should be created
/// * `major` - The major device number
/// * `minor` - The minor device number
#[cfg(unix)]
pub fn make_char_device(path: &Path, major: u32, minor: u32) -> std::io::Result<()> {
let name = CString::new(path.to_str().unwrap()).unwrap();
let dev = makedev(major, minor);
let err = unsafe { mknod(name.as_ptr(), S_IFCHR | 0o666, dev) };
if err == -1 {
Err(std::io::Error::last_os_error())
} else {
Ok(())
}
}

/// Create a block device file.
///
/// # Arguments
/// * `path` - The path where the device file should be created
/// * `major` - The major device number
/// * `minor` - The minor device number
#[cfg(unix)]
pub fn make_block_device(path: &Path, major: u32, minor: u32) -> std::io::Result<()> {
let name = CString::new(path.to_str().unwrap()).unwrap();
let dev = makedev(major, minor);
let err = unsafe { mknod(name.as_ptr(), S_IFBLK | 0o666, dev) };
if err == -1 {
Err(std::io::Error::last_os_error())
} else {
Ok(())
}
}

#[cfg(test)]
mod tests {
// Note this useful idiom: importing names from outer (for mod tests) scope.
Expand Down
Loading
Loading