Fix regression: Allow FIFO / process substitution for --read-batch

[seks99x]

V3.5.0 in batch.c commit 1604890 introduced a strict S_ISREG check for --read-batch argument paths. This inadvertently breaks bash process substitution (e.g., <(...)), which passes file descriptors as FIFOs (S_IFIFO).

Testing against the 3.4 branch succeeds, but fails on the current 3.5.0dev branch:

Bash

# rsync 3.4.x (Success)
./rsync --read-batch=<(cat reg) /tmp/rsync_test_dir/src/test
skipping non-regular file "test"

# rsync 3.5.0dev (Regression)
./rsync --read-batch=<(cat reg) /tmp/rsync_test_dir/src/test 
Batch file "/dev/fd/63" is not a regular file
rsync error: error in file IO (code 11) at batch.c(281) [Receiver=3.5.0dev-ge8c79d2d]

The Fix:
Modified the check in batch.c open_batch_file() to permit S_ISFIFO alongside S_ISREG.

[seks99x]

the PR wont pass the CI without merging PR #1054

[seks99x]

Actually this would still break on a socket:[ / anon_inode: case. We are only allowing pipes pipe:[.

I think we should not strict a certain file type and only check for do_fstat(batch_fd, &st) == 0 .
@steadytao what do you think?

[steadytao]

Sorry for the late response. Very busy weekend. Best to keep this limited to regular files and FIFOs. Sockets, devices and anonymous inodes are outside the process-substitution use case and removing the type check would undo the original hardening. The test should also clear dest after generating the batch so the read-batch run itself has to recreate the payload. Rebase this after #1054 is resolved of course.

[seks99x]

@steadytao Unfortunately we could break a legit case or just receive another regression issue if we strict the file type. Sockets can be used also with certain shell types like KornShell. Do you have any specific reason why we should strict the file type here on the read-batch?
Take your time mate no worries!

[steadytao]

If KornShell produces a socket for a real --read-batch invocation, please provide the exact shell version and reproducer so we can test that case. Without one, I merely do not want to remove the type restriction and allow every object that happens to pass fstat(). The existing FIFO/process-substitution regression is concrete; sockets and anonymous inodes currently are not.

The test also needs to remove the destination after generating the batch so the --read-batch operation must recreate it.

[steadytao]

Trying to be minimal but that could be a concern so perhaps some testing is justified?

[seks99x]

@steadytao Actually, I just tested KornShell and it uses standard pipes (pipe:[). Looking at the man pages, it says it uses normal pipes.
https://cdesktopenv.sourceforge.io/man1/ksh.html

but on ksh93 it may uses socketpair instead on pipeline not process substitution
https://manpages.debian.org/unstable/ksh93u+m/ksh93.1.en.html

A pipeline is a sequence of one or more commands separated by |. The standard output of each command but the last is connected by a [socketpair(2)](https://manpages.debian.org/unstable/manpages-dev/socketpair.2.en.html) or (if the posix shell option is on) by a [pipe(2)](https://manpages.debian.org/unstable/manpages-dev/pipe.2.en.html) to the standard input of the next command. Each command except the last is run asynchronously in a subshell (see Subshells below). If the monitor or pipefail option is on, or the pipeline is preceded by the reserved word time, then the shell waits for all component commands in the pipeline to terminate; otherwise, the shell only waits for the last component command. The exit status of a pipeline is the exit status of its last component command, unless the pipefail option is enabled. Each pipeline can be preceded by the reserved word ! which causes the exit status of the pipeline to become 0 if the exit status of the last command is non-zero, and 1 if the exit status of the last command is 0.

#set +o posix
# sleep 60 | sleep 61 &                                                                                             
[1]	438284
# ls -l /proc/$!/fd/0                                                                                               
lrwx------ 1 root root 64 Aug 25 14:39 /proc/438284/fd/0 -> 'socket:[6649015]'
# ls -l /proc/$!/fd/ 
total 0
lrwx------ 1 root root 64 Aug 25 14:39 0 -> 'socket:[6649015]'
lrwx------ 1 root root 64 Aug 25 14:39 1 -> /dev/pts/4
lrwx------ 1 root root 64 Aug 25 14:39 10 -> /root/.sh_history
lrwx------ 1 root root 64 Aug 25 14:39 11 -> /dev/pts/4
lrwx------ 1 root root 64 Aug 25 14:39 2 -> /dev/pts/4
# 

But this will be handled well without adding anything in the source due to this check if (!write_batch && batch_fd != STDIN_FILENO) { if we sent a rsync cli like this cat /tmp/mybatch | rsync -av --read-batch=- /tmp/rsync_dest2/ it will succeed normally because it will use standard input.

The only thing could fail is things like sockets or anon_inodes that won't come directly from standard bash process substitution and is being explicitly prepared first.

I think we better keep it stricter to FIFOs if a legit case of socket is already handled.

[seks99x]

I'll rebase and fix the test

[tridge]

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report (with the other AIReview PR): https://uav.tridgell.net/RsyncReviews/index.html#pr1060

Reviewed at head 0580585747 — verdict: REQUEST CHANGES. The direction is right — relaxing the type check to S_ISREG || S_ISFIFO (still rejecting sockets/devices) is what steadytao asked for, and there's no seek dependency on batch_fd (it's read sequentially by the normal protocol reader), so a FIFO is functionally fine. But the PR isn't mergeable yet:

• CI is red because of this PR's own new test, and it can't go green alone. The read-batch-pipe test fails at batch.c:271 with Batch file "/dev/fd/63" open error: No such file or directory — the open fails before the type check, because two 3.5.0dev hardening additions block a process-substitution batch: (a) open_no_attacker_symlinks() returns ENOENT on /dev/fd/63, and (b) the S_ISREG check. This PR only fixes (b); (a) is the unmerged Fix ENOENT when resolving kernel pseudo-paths in ona_open #1054. So this needs to rebase on / stack after Fix ENOENT when resolving kernel pseudo-paths in ona_open #1054 to pass CI (you already noted this in the thread). The AlmaLinux-8 and Cygwin reds are skiplist mismatches, not code (below).
• BUG — the new test is self-passing. --write-batch already writes dest/payload.txt, and dest isn't cleared before the --read-batch run, so the final is_file() assertion passes even if replay does nothing. (Confirmed by reproduction — replacing replay with a no-op still passes.) Clear/recreate dest between the two runs so the test actually exercises the fix.
• ISSUE — skiplists: add the test to almalinux-8.txt (it self-skips there for lack of bash procsub → expected/got mismatch) and remove it from cygwin.txt (it runs and passes there → mismatch).
• NOTE — style: batch.c:277 adds a blank line containing a tab (trailing whitespace); the batch.c:276 comment reads garbled; commit 2 has an empty body.

Memory-safety / untrusted-input: no concern — it's a one-condition change with no new read loop or length handling.

[steadytao]

LGTM. Rebase after 1054.