Close original standard descriptors and keep C++ streams attached

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Tomás Senart <ts@perfloop.ai>

Author: perfloop-agent

docs/src/runtime/io.md

@@ -42,14 +42,20 @@ through the `CFile`; the unsafe variant forwards to `libc::fread`.
 ## C++ Streams
 
 In the refcount model `cin`, `cout`, and `cerr` are translated as
-`Ptr<std::fs::File>` values over duplicates of the standard descriptors,
-returned by the `cin`, `cout`, and `cerr` functions. `Ptr<T>` implements
-`write_fmt` and `write_all` whenever `T: Write`, forwarding to the pointee
-through `with_mut`, so `cout << x` becomes `write!(cout(), "{}", x)` and a raw
-byte range is written with `cout().write_all(..)`. In the unsafe model
-`cin_unsafe`, `cout_unsafe`, and `cerr_unsafe` return raw pointers to
-thread-local `std::fs::File` values. C++ streams do not map fully onto
-`std::fs::File`, so this translation may change in the future.
+`Ptr<CppStream>` values returned by the `cin`, `cout`, and `cerr` functions.
+`CppStream` uses the original standard descriptor number without owning or
+duplicating it. Its unbuffered reads and writes return an I/O error if the
+descriptor is closed, even when the stream is first used after the close. File
+streams use the same type, wrapping an owned `std::fs::File`; cloning these
+streams still duplicates the owned file handle.
+
+`Ptr<T>` implements `write_fmt` and `write_all` whenever `T: Write`, forwarding
+to the pointee through `with_mut`, so `cout << x` becomes
+`write!(cout(), "{}", x)` and a raw byte range is written with
+`cout().write_all(..)`. In the unsafe model `cin_unsafe`, `cout_unsafe`, and
+`cerr_unsafe` return raw pointers to thread-local `CppStream` values. The
+translation does not yet model the full C++ iostream state, including sticky
+error flags, and insertion expressions discard the returned I/O error.
 
 ## Formatting
 
@@ -94,8 +100,11 @@ The registry follows the descriptor's life. When a rule opens a file,
 raw number. When a rule performs I/O on that number, `FdRegistry::with_fd` looks
 the entry up and lends it out as a `BorrowedFd` for the duration of the call.
 When the program calls `close`, `FdRegistry::close` removes the entry, which
-closes the descriptor. The registry starts out holding the standard descriptors
-0, 1, and 2.
+closes the descriptor. The registry starts out borrowing the live standard
+descriptors 0, 1, and 2 from Rust's standard stream handles, without duplicating
+them. An explicit `close` releases the original descriptor, but dropping the
+thread-local registry does not close these borrowed standard descriptors. A file
+opened at one of these numbers is owned and closed normally.
 
 In the `fstat` rule, the descriptor argument goes through `with_fd`:
 
@@ -127,4 +136,8 @@ FdRegistry::with_fds(&wanted, |borrowed: &[BorrowedFd<'_>]| {
 
 Using a descriptor that was never opened, or using it after it was closed, is a
 bug in the original program. The registry turns such a use into a panic (with a
-message prefixed `ub:`) so the bug surfaces instead of going unnoticed.
+message prefixed `ub:`) so the bug surfaces instead of going unnoticed. This
+policy also applies when another thread has already closed a standard descriptor
+before this thread initializes its registry. The table remains thread-local: it
+does not track descriptor ownership or closure across threads, and does not
+provide native C's `EBADF` behavior for unregistered numbers.

libcc2rs/src/fd.rs

@@ -4,17 +4,51 @@
 use std::cell::RefCell;
 use std::os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd};
 
+enum Fd {
+    Owned(OwnedFd),
+    Stdin(std::io::Stdin),
+    Stdout(std::io::Stdout),
+    Stderr(std::io::Stderr),
+}
+
+impl AsRawFd for Fd {
+    fn as_raw_fd(&self) -> i32 {
+        match self {
+            Self::Owned(fd) => fd.as_raw_fd(),
+            Self::Stdin(fd) => fd.as_raw_fd(),
+            Self::Stdout(fd) => fd.as_raw_fd(),
+            Self::Stderr(fd) => fd.as_raw_fd(),
+        }
+    }
+}
+
+impl AsFd for Fd {
+    fn as_fd(&self) -> BorrowedFd<'_> {
+        match self {
+            Self::Owned(fd) => fd.as_fd(),
+            Self::Stdin(fd) => fd.as_fd(),
+            Self::Stdout(fd) => fd.as_fd(),
+            Self::Stderr(fd) => fd.as_fd(),
+        }
+    }
+}
+
 pub struct FdRegistry {
-    fds: Vec<Option<OwnedFd>>,
+    fds: Vec<Option<Fd>>,
 }
 
 thread_local! {
     static FD_REGISTRY: RefCell<FdRegistry> = RefCell::new(FdRegistry {
-        fds: vec![
-            std::io::stdin().as_fd().try_clone_to_owned().ok(),
-            std::io::stdout().as_fd().try_clone_to_owned().ok(),
-            std::io::stderr().as_fd().try_clone_to_owned().ok(),
-        ],
+        // Borrow the original standard descriptors, without closing them when
+        // this thread exits. Only an explicit close releases these descriptors.
+        fds: [
+            Fd::Stdin(std::io::stdin()),
+            Fd::Stdout(std::io::stdout()),
+            Fd::Stderr(std::io::stderr()),
+        ].into_iter().map(|fd| {
+            // Check the number without borrowing a potentially closed descriptor.
+            (unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_GETFD) } >= 0).then_some(fd)
+        }).collect(),
     });
 }
 
@@ -27,8 +61,13 @@ impl FdRegistry {
             if fds.len() <= idx {
                 fds.resize_with(idx + 1, || None);
             }
-            assert!(fds[idx].is_none(), "ub: fd registry collision on fd {raw}");
-            fds[idx] = Some(fd);
+            // The first open may reuse an initially closed standard descriptor
+            // before the registry is initialized. Replace its borrowed entry.
+            assert!(
+                !matches!(fds[idx], Some(Fd::Owned(_))),
+                "ub: fd registry collision on fd {raw}"
+            );
+            fds[idx] = Some(Fd::Owned(fd));
         });
         raw
     }
@@ -57,13 +96,22 @@ impl FdRegistry {
     }
 
     pub fn close(fd: i32) -> i32 {
-        FD_REGISTRY.with(|r| {
+        let entry = FD_REGISTRY.with(|r| {
             r.borrow_mut()
                 .fds
                 .get_mut(fd as usize)
                 .and_then(|slot| slot.take())
                 .unwrap_or_else(|| panic!("ub: bad fd {fd}"))
         });
+        match entry {
+            Fd::Owned(fd) => drop(fd),
+            fd => {
+                if let Err(e) = nix::unistd::close(fd.as_raw_fd()) {
+                    crate::cpp2rust_errno().write(e as i32);
+                    return -1;
+                }
+            }
+        }
         0
     }
 }

libcc2rs/src/io.rs

@@ -3,49 +3,92 @@
 
 use crate::{AnyPtr, AsPointer, CFile, Ptr, Value};
 use std::cell::{RefCell, UnsafeCell};
-use std::os::fd::{AsFd, FromRawFd, IntoRawFd};
+use std::io::{self, Read, Write};
 use std::rc::Rc;
 
+enum StreamHandle {
+    File(std::fs::File),
+    Standard(i32),
+}
+
+pub struct CppStream(StreamHandle);
+
+impl From<std::fs::File> for CppStream {
+    fn from(file: std::fs::File) -> Self {
+        Self(StreamHandle::File(file))
+    }
+}
+
+impl CppStream {
+    fn standard(fd: i32) -> Self {
+        Self(StreamHandle::Standard(fd))
+    }
+
+    pub fn try_clone(&self) -> io::Result<Self> {
+        match &self.0 {
+            StreamHandle::File(file) => file.try_clone().map(Self::from),
+            StreamHandle::Standard(fd) => Ok(Self::standard(*fd)),
+        }
+    }
+}
+
+impl Read for &CppStream {
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
+        match &self.0 {
+            StreamHandle::File(file) => (&*file).read(buf),
+            StreamHandle::Standard(fd) => {
+                // A standard descriptor may have been closed. Do not create an
+                // owning File or a BorrowedFd, which requires an open descriptor.
+                let n = unsafe { libc::read(*fd, buf.as_mut_ptr().cast(), buf.len()) };
+                if n < 0 {
+                    Err(io::Error::last_os_error())
+                } else {
+                    Ok(n as usize)
+                }
+            }
+        }
+    }
+}
+
+impl Read for CppStream {
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
+        (&*self).read(buf)
+    }
+}
+
+impl Write for CppStream {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        match &mut self.0 {
+            StreamHandle::File(file) => file.write(buf),
+            StreamHandle::Standard(fd) => {
+                // The slice is valid for the duration of this unbuffered write.
+                let n = unsafe { libc::write(*fd, buf.as_ptr().cast(), buf.len()) };
+                if n < 0 {
+                    Err(io::Error::last_os_error())
+                } else {
+                    Ok(n as usize)
+                }
+            }
+        }
+    }
+
+    fn flush(&mut self) -> io::Result<()> {
+        match &mut self.0 {
+            StreamHandle::File(file) => file.flush(),
+            StreamHandle::Standard(_) => Ok(()),
+        }
+    }
+}
+
+impl crate::ByteRepr for CppStream {}
+
 thread_local! {
-    static SAFE_STDIN: Value<std::fs::File> = Rc::new(RefCell::new(std::fs::File::from(
-        std::io::stdin().as_fd().try_clone_to_owned().unwrap(),
-    )));
-    static SAFE_STDOUT: Value<std::fs::File> = Rc::new(RefCell::new(std::fs::File::from(
-        std::io::stdout().as_fd().try_clone_to_owned().unwrap(),
-    )));
-    static SAFE_STDERR: Value<std::fs::File> = Rc::new(RefCell::new(std::fs::File::from(
-        std::io::stderr().as_fd().try_clone_to_owned().unwrap(),
-    )));
-    static UNSAFE_STDIN: UnsafeCell<std::fs::File> = unsafe {
-        UnsafeCell::new(
-            std::fs::File::from_raw_fd(
-                std::io::stdin()
-                    .as_fd()
-                    .try_clone_to_owned()
-                    .unwrap()
-                    .into_raw_fd(),
-        ))
-    };
-    static UNSAFE_STDOUT: UnsafeCell<std::fs::File> = unsafe {
-        UnsafeCell::new(
-            std::fs::File::from_raw_fd(
-                std::io::stdout()
-                    .as_fd()
-                    .try_clone_to_owned()
-                    .unwrap()
-                    .into_raw_fd(),
-        ))
-    };
-    static UNSAFE_STDERR: UnsafeCell<std::fs::File> = unsafe {
-        UnsafeCell::new(
-            std::fs::File::from_raw_fd(
-                std::io::stderr()
-                    .as_fd()
-                    .try_clone_to_owned()
-                    .unwrap()
-                    .into_raw_fd(),
-        ))
-    };
+    static SAFE_STDIN: Value<CppStream> = Rc::new(RefCell::new(CppStream::standard(0)));
+    static SAFE_STDOUT: Value<CppStream> = Rc::new(RefCell::new(CppStream::standard(1)));
+    static SAFE_STDERR: Value<CppStream> = Rc::new(RefCell::new(CppStream::standard(2)));
+    static UNSAFE_STDIN: UnsafeCell<CppStream> = UnsafeCell::new(CppStream::standard(0));
+    static UNSAFE_STDOUT: UnsafeCell<CppStream> = UnsafeCell::new(CppStream::standard(1));
+    static UNSAFE_STDERR: UnsafeCell<CppStream> = UnsafeCell::new(CppStream::standard(2));
 }
 
 thread_local! {
@@ -66,39 +109,39 @@ pub fn c_stderr() -> Ptr<CFile> {
     C_STDERR.with(AsPointer::as_pointer)
 }
 
-pub fn cin() -> Ptr<std::fs::File> {
+pub fn cin() -> Ptr<CppStream> {
     SAFE_STDIN.with(AsPointer::as_pointer)
 }
 
-pub fn cout() -> Ptr<std::fs::File> {
+pub fn cout() -> Ptr<CppStream> {
     SAFE_STDOUT.with(AsPointer::as_pointer)
 }
 
-pub fn cerr() -> Ptr<std::fs::File> {
+pub fn cerr() -> Ptr<CppStream> {
     SAFE_STDERR.with(AsPointer::as_pointer)
 }
 
 /// # Safety
 ///
 /// The caller must ensure that the returned pointer is not used after the
 /// thread finishes.
-pub unsafe fn cin_unsafe() -> *mut std::fs::File {
+pub unsafe fn cin_unsafe() -> *mut CppStream {
     UNSAFE_STDIN.with(UnsafeCell::get)
 }
 
 /// # Safety
 ///
 /// The caller must ensure that the returned pointer is not used after the
 /// thread finishes.
-pub unsafe fn cout_unsafe() -> *mut std::fs::File {
+pub unsafe fn cout_unsafe() -> *mut CppStream {
     UNSAFE_STDOUT.with(UnsafeCell::get)
 }
 
 /// # Safety
 ///
 /// The caller must ensure that the returned pointer is not used after the
 /// thread finishes.
-pub unsafe fn cerr_unsafe() -> *mut std::fs::File {
+pub unsafe fn cerr_unsafe() -> *mut CppStream {
     UNSAFE_STDERR.with(UnsafeCell::get)
 }
 
@@ -189,3 +232,27 @@ pub unsafe fn fread_unsafe(
 ) -> usize {
     unsafe { libc::fread(a0, a1, a2, a3) }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn file_stream_io() {
+        let (read, write) = nix::unistd::pipe().unwrap();
+        let mut input = CppStream::from(std::fs::File::from(read));
+        let output = CppStream::from(std::fs::File::from(write));
+        let mut copy = output.try_clone().unwrap();
+        drop(output);
+        copy.write_all(&[0, 127, 255]).unwrap();
+        copy.flush().unwrap();
+        drop(copy);
+
+        let mut first = [9];
+        input.read_exact(&mut first).unwrap();
+        assert_eq!(first, [0]);
+        let mut rest = Vec::new();
+        (&input).read_to_end(&mut rest).unwrap();
+        assert_eq!(rest, [127, 255]);
+    }
+}

rules/algorithm/tgt_refcount.rs

@@ -119,7 +119,11 @@ fn f12<T1: CppInputIterator<T2>, T2: Clone + ByteRepr>(a0: T1, a1: T1, a2: T2) {
     }
 }
 
-fn f13<T1: CppInputIterator<i8>>(a0: T1, a1: T1, a2: &mut ::std::fs::File) -> ::std::fs::File {
+fn f13<T1: CppInputIterator<i8>>(
+    a0: T1,
+    a1: T1,
+    a2: &mut libcc2rs::CppStream,
+) -> libcc2rs::CppStream {
     let mut __it = a0;
     let __last = a1;
     let __out = &mut *a2;

rules/algorithm/tgt_unsafe.rs

@@ -127,8 +127,8 @@ unsafe fn f12<T1: UnsafeCppInputIterator<T2>, T2: Clone>(a0: T1, a1: T1, a2: T2)
 unsafe fn f13<T1: UnsafeCppInputIterator<libc::c_char>>(
     a0: T1,
     a1: T1,
-    a2: &mut ::std::fs::File,
-) -> ::std::fs::File {
+    a2: &mut libcc2rs::CppStream,
+) -> libcc2rs::CppStream {
     let mut __it = a0;
     let __last = a1;
     let __out = &mut *a2;