Skip to content

fix(mv): prevent hang when moving directories containing FIFO files - #9665

Open
naoNao89 wants to merge 4 commits into
uutils:mainfrom
naoNao89:fix/mv-fifo-hang-9656
Open

naoNao89 wants to merge 4 commits into
uutils:mainfrom
naoNao89:fix/mv-fifo-hang-9656

Conversation

@naoNao89

Copy link
Copy Markdown
Contributor

Fixes #9656

Adds FIFO detection in copy_dir_contents_recursive() to prevent indefinite hang when moving directories containing FIFO files. Uses make_fifo() instead of fs::copy().

Includes test with timeout protection

@naoNao89
naoNao89 force-pushed the fix/mv-fifo-hang-9656 branch from 27b0ce0 to 6ec985d Compare December 15, 2025 14:57
@oech3

oech3 commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

Is your test really cross-filesystem? See discussion at #8605 (comment) too.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@naoNao89
naoNao89 force-pushed the fix/mv-fifo-hang-9656 branch from 6ec985d to f7ef980 Compare December 15, 2025 15:19
@naoNao89

Copy link
Copy Markdown
Contributor Author

okey
test_mv_dir_containing_fifo_cross_filesystem() moving across filesystems to /dev/shm (the scenario where the bug occurs). #[ignore] since it requires /dev/shm.

test_mv_dir_containing_fifo_same_filesystem()tests the normal case on the same filesystem.

these ensures FIFO handling works in both scenarios

@oech3

oech3 commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

Can we remove ignore and just skip on th system /dev/shm is unusable? #8605 is not merged yet because it has ignore.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@naoNao89
naoNao89 force-pushed the fix/mv-fifo-hang-9656 branch from f7ef980 to 24b49c3 Compare December 15, 2025 15:39
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@oech3

oech3 commented Dec 15, 2025 •

Copy link
Copy Markdown
Contributor

I guess non-cross FS test was added at #7241 . So we need test_mv_dir_containing_fifo_cross_filesystem only at here.

@oech3

oech3 commented Dec 15, 2025 •

Copy link
Copy Markdown
Contributor

Oh it has ignore...

@naoNao89

Copy link
Copy Markdown
Contributor Author

heh, you want proper test but that require root. something like:

#[cfg(unix)]
#[ignore = "requires root for mount"]
#[test]
fn test_mv_dir_with_fifo_cross_filesystem() {
    let mut ts = TestScenario::new(util_name!());
    let at = &ts.fixtures;
    
    at.mkdir("source");
    at.mkfifo("source/pipe");
    at.mkdir("mountpoint");
    
    ts.mount_temp_fs(at.plus_as_string("mountpoint")).unwrap();
    
    ts.ucmd()
        .args(&["source", "mountpoint/dest"])
        .timeout(Duration::from_secs(2))
        .succeeds();
}

@naoNao89
naoNao89 force-pushed the fix/mv-fifo-hang-9656 branch from d1455d6 to 24b49c3 Compare December 15, 2025 21:45
@oech3

oech3 commented Dec 15, 2025 via email •

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@oech3

oech3 commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

I guess we can reuse cp -a's codebase and fix fifo,symlink,special,etc... without many if (not sure)...

@naoNao89

Copy link
Copy Markdown
Contributor Author

But that's a big refactor, reusing cp's file type handling would touch many parts of mv. You should create a new issue as enhancement: "mv: share special file handling logic with cp"

Comment thread src/uu/mv/src/mv.rs Outdated
Fixes uutils#9656

When moving a directory containing FIFO files, copy_dir_contents_recursive()
would call fs::copy() on the FIFO, causing an indefinite hang. This fix adds
FIFO detection before the copy operation and uses make_fifo() instead, matching
the behavior of rename_fifo_fallback().

- Add FIFO detection in copy_dir_contents_recursive()
- Create new FIFO at destination using make_fifo()
- Add test with timeout protection to prevent future regressions
@naoNao89
naoNao89 force-pushed the fix/mv-fifo-hang-9656 branch from 24b49c3 to 1e5bf34 Compare December 19, 2025 21:44
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@codspeed

codspeed Bot commented Dec 19, 2025 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 126 untouched benchmarks
⏩ 6 skipped benchmarks1


Comparing naoNao89:fix/mv-fifo-hang-9656 (acfd80d) with main (f43602d)

Open in CodSpeed

Footnotes

  1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@oech3

oech3 commented Dec 20, 2025

Copy link
Copy Markdown
Contributor

To simplify tests: #8605 (comment)

- Add macOS mount_temp_fs() implementation using hdiutil ramdisk
- Refactor test_mv_dir_containing_fifo to use TestScenario infrastructure
- Replace platform-specific /dev/shm dependency with portable mount_temp_fs()
- Consolidate separate cross-fs and same-fs tests into single test
- Add root privilege requirement check
- Implement automatic cleanup via Drop trait on all platforms

This completes cross-platform test infrastructure:
- Linux/FreeBSD/Android: tmpfs mounting (already existed)
- macOS: ramdisk creation + HFS+ format + mount (new)

Both implementations provide mount_temp_fs()/umount_temp_fs() for
cross-device testing without relying on system-specific paths.
@naoNao89
naoNao89 force-pushed the fix/mv-fifo-hang-9656 branch from b9d9f2b to 68a35de Compare December 20, 2025 23:01
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/basenc/bounded-memory is now passing!

Fixes compilation error on OpenBSD CI by extending platform-specific
conditional compilation to include target_os = 'openbsd'.

OpenBSD supports tmpfs mounting using the same mount -t tmpfs command
as Linux and FreeBSD, so the existing implementation works without
modification.

This resolves the failing OpenBSD CI job for PR uutils#9665 which was
failing with 'no method named mount_temp_fs found' errors.
@naoNao89
naoNao89 force-pushed the fix/mv-fifo-hang-9656 branch from 3924c18 to acfd80d Compare December 30, 2025 11:57

This branch has not been deployed

No deployments
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.

mv: mkdir a;mkfifo a/f;mv a /tmp/a hangs

3 participants