Auto merge of rust-lang#108283 - the8472:remove-splice-into-pipe, r=ChrisDenton

don't splice from files into pipes in io::copy This fixes potential data ordering issue where a write performed after a copy operation could become visible in the copy even though it signaled completion. I assumed that by not setting `SPLICE_F_MOVE` we would be safe and the kernel would do a copy in kernel space and we could avoid the read-write syscall and copy-to/from-userspace costs. But apparently that flag only makes a difference when splicing from a pipe, but not when splicing into it. Context: https://lkml.org/lkml/2023/2/9/673

Author: bors

library/std/src/sys/unix/kernel_copy.rs

@@ -17,11 +17,9 @@
 //! Once it has obtained all necessary pieces and brought any wrapper types into a state where they
 //! can be safely bypassed it will attempt to use the `copy_file_range(2)`,
 //! `sendfile(2)` or `splice(2)` syscalls to move data directly between file descriptors.
-//! Since those syscalls have requirements that cannot be fully checked in advance and
-//! gathering additional information about file descriptors would require additional syscalls
-//! anyway it simply attempts to use them one after another (guided by inaccurate hints) to
-//! figure out which one works and falls back to the generic read-write copy loop if none of them
-//! does.
+//! Since those syscalls have requirements that cannot be fully checked in advance it attempts
+//! to use them one after another (guided by hints) to figure out which one works and
+//! falls back to the generic read-write copy loop if none of them does.
 //! Once a working syscall is found for a pair of file descriptors it will be called in a loop
 //! until the copy operation is completed.
 //!
@@ -84,14 +82,10 @@ pub(crate) fn copy_spec<R: Read + ?Sized, W: Write + ?Sized>(
 /// The methods on this type only provide hints, due to `AsRawFd` and `FromRawFd` the inferred
 /// type may be wrong.
 enum FdMeta {
- /// We obtained the FD from a type that can contain any type of `FileType` and queried the metadata
- /// because it is cheaper than probing all possible syscalls (reader side)
  Metadata(Metadata),
  Socket,
  Pipe,
- /// We don't have any metadata, e.g. because the original type was `File` which can represent
- /// any `FileType` and we did not query the metadata either since it did not seem beneficial
- /// (writer side)
+ /// We don't have any metadata because the stat syscall failed
  NoneObtained,
 }
 
@@ -131,6 +125,39 @@ impl FdMeta {
  }
 }
 
+/// Returns true either if changes made to the source after a sendfile/splice call won't become
+/// visible in the sink or the source has explicitly opted into such behavior (e.g. by splicing
+/// a file into a pipe, the pipe being the source in this case).
+///
+/// This will prevent File -> Pipe and File -> Socket splicing/sendfile optimizations to uphold
+/// the Read/Write API semantics of io::copy.
+///
+/// Note: This is not 100% airtight, the caller can use the RawFd conversion methods to turn a
+/// regular file into a TcpSocket which will be treated as a socket here without checking.
+fn safe_kernel_copy(source: &FdMeta, sink: &FdMeta) -> bool {
+ match (source, sink) {
+ // Data arriving from a socket is safe because the sender can't modify the socket buffer.
+ // Data arriving from a pipe is safe(-ish) because either the sender *copied*
+ // the bytes into the pipe OR explicitly performed an operation that enables zero-copy,
+ // thus promising not to modify the data later.
+ (FdMeta::Socket, _) => true,
+ (FdMeta::Pipe, _) => true,
+ (FdMeta::Metadata(meta), _)
+ if meta.file_type().is_fifo() || meta.file_type().is_socket() =>
+ {
+ true
+ }
+ // Data going into non-pipes/non-sockets is safe because the "later changes may become visible" issue
+ // only happens for pages sitting in send buffers or pipes.
+ (_, FdMeta::Metadata(meta))
+ if !meta.file_type().is_fifo() && !meta.file_type().is_socket() =>
+ {
+ true
+ }
+ _ => false,
+ }
+}
+
 struct CopyParams(FdMeta, Option<RawFd>);
 
 struct Copier<'a, 'b, R: Read + ?Sized, W: Write + ?Sized> {
@@ -186,7 +213,8 @@ impl<R: CopyRead, W: CopyWrite> SpecCopy for Copier<'_, '_, R, W> {
  // So we just try and fallback if needed.
  // If current file offsets + write sizes overflow it may also fail, we do not try to fix that and instead
  // fall back to the generic copy loop.
- if input_meta.potential_sendfile_source() {
+ if input_meta.potential_sendfile_source() && safe_kernel_copy(&input_meta, &output_meta)
+ {
  let result = sendfile_splice(SpliceMode::Sendfile, readfd, writefd, max_write);
  result.update_take(reader);
 
@@ -197,7 +225,9 @@ impl<R: CopyRead, W: CopyWrite> SpecCopy for Copier<'_, '_, R, W> {
  }
  }
 
- if input_meta.maybe_fifo() || output_meta.maybe_fifo() {
+ if (input_meta.maybe_fifo() || output_meta.maybe_fifo())
+ && safe_kernel_copy(&input_meta, &output_meta)
+ {
  let result = sendfile_splice(SpliceMode::Splice, readfd, writefd, max_write);
  result.update_take(reader);
 
@@ -298,13 +328,13 @@ impl CopyRead for &File {
 
 impl CopyWrite for File {
  fn properties(&self) -> CopyParams {
- CopyParams(FdMeta::NoneObtained, Some(self.as_raw_fd()))
+ CopyParams(fd_to_meta(self), Some(self.as_raw_fd()))
  }
 }
 
 impl CopyWrite for &File {
  fn properties(&self) -> CopyParams {
- CopyParams(FdMeta::NoneObtained, Some(self.as_raw_fd()))
+ CopyParams(fd_to_meta(*self), Some(self.as_raw_fd()))
  }
 }
 
@@ -401,13 +431,13 @@ impl CopyRead for StdinLock<'_> {
 
 impl CopyWrite for StdoutLock<'_> {
  fn properties(&self) -> CopyParams {
- CopyParams(FdMeta::NoneObtained, Some(self.as_raw_fd()))
+ CopyParams(fd_to_meta(self), Some(self.as_raw_fd()))
  }
 }
 
 impl CopyWrite for StderrLock<'_> {
  fn properties(&self) -> CopyParams {
- CopyParams(FdMeta::NoneObtained, Some(self.as_raw_fd()))
+ CopyParams(fd_to_meta(self), Some(self.as_raw_fd()))
  }
 }
 

library/std/src/sys/unix/kernel_copy/tests.rs

@@ -83,6 +83,48 @@ fn copies_append_mode_sink() -> Result<()> {
  Ok(())
 }
 
+#[test]
+fn dont_splice_pipes_from_files() -> Result<()> {
+ // splicing to a pipe and then modifying the source could lead to changes
+ // becoming visible in an unexpected order.
+
+ use crate::io::SeekFrom;
+ use crate::os::unix::fs::FileExt;
+ use crate::process::{ChildStdin, ChildStdout};
+ use crate::sys_common::FromInner;
+
+ let (read_end, write_end) = crate::sys::pipe::anon_pipe()?;
+
+ let mut read_end = ChildStdout::from_inner(read_end);
+ let mut write_end = ChildStdin::from_inner(write_end);
+
+ let tmp_path = tmpdir();
+ let file = tmp_path.join("to_be_modified");
+ let mut file =
+ crate::fs::OpenOptions::new().create_new(true).read(true).write(true).open(file)?;
+
+ const SZ: usize = libc::PIPE_BUF as usize;
+
+ // put data in page cache
+ let mut buf: [u8; SZ] = [0x01; SZ];
+ file.write_all(&buf).unwrap();
+
+ // copy page into pipe
+ file.seek(SeekFrom::Start(0)).unwrap();
+ assert!(io::copy(&mut file, &mut write_end).unwrap() == SZ as u64);
+
+ // modify file
+ buf[0] = 0x02;
+ file.write_at(&buf, 0).unwrap();
+
+ // read from pipe
+ read_end.read_exact(buf.as_mut_slice()).unwrap();
+
+ assert_eq!(buf[0], 0x01, "data in pipe should reflect the original, not later modifications");
+
+ Ok(())
+}
+
 #[bench]
 fn bench_file_to_file_copy(b: &mut test::Bencher) {
  const BYTES: usize = 128 * 1024;