fix fd leak on Linux using libuv (#841)

Closes #757 

Co-authored-by: Aviram Hassan <[email protected]>

.github/workflows/ci.yaml

@@ -169,26 +169,6 @@ jobs:
       - run: |
           cargo test -p mirrord-sip
 
-  test_mirrord_layer:
-    strategy:
-      matrix:
-        os: [ubuntu-latest]
-    runs-on: ${{ matrix.os }}
-    needs: changed_files
-    if: ${{needs.changed_files.outputs.rs_changed == 'true'}}
-    steps:
-      - uses: actions/checkout@v2
-      - uses: actions-rs/toolchain@v1
-        with:
-          profile: minimal
-          toolchain: nightly
-          target: ${{matrix.target}}
-      - uses: Swatinem/rust-cache@v2
-        with:
-          key: ${{matrix.os}}
-      - run: |
-          cargo test -p mirrord-layer -- --skip test_self_open --skip test_pwrite --skip test_mirroring_with_http --skip test_self_connect
-
   test_agent:
     runs-on: ubuntu-latest
     needs: changed_files
@@ -499,7 +479,6 @@ jobs:
         lint,
         lint_macos,
         test_mirrord_config,
-        test_mirrord_layer,
         test_mirrord_protocol,
         test_mirrord_sip,
         build_intellij_plugin,
@@ -519,7 +498,6 @@ jobs:
       (needs.lint.result == 'success' || needs.lint.result == 'skipped') &&
       (needs.lint_macos.result == 'success' || needs.lint_macos.result == 'skipped') &&
       (needs.test_mirrord_config.result == 'success' || needs.test_mirrord_config.result == 'skipped') &&
-      (needs.test_mirrord_layer.result == 'success' || needs.test_mirrord_layer.result == 'skipped') &&
       (needs.test_mirrord_protocol.result == 'success' || needs.test_mirrord_protocol.result == 'skipped') &&
       (needs.test_mirrord_sip.result == 'success' || needs.test_mirrord_sip.result == 'skipped') &&
       (needs.build_intellij_plugin.result == 'success' || needs.build_intellij_plugin.result == 'skipped') &&

CHANGELOG.md

@@ -12,6 +12,12 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how
 - Don't add temp prefix when using `extract` command.
 - VS Code extension: mirrord enable/disable to be per workspace.
 - VS Code extension: bundle the resources
+- Add `/System` to default ignore list.
+- Remove `test_mirrord_layer` from CI as it's covered in integration testing.
+
+### Fixed
+
+- fd leak on Linux when using libuv (Node). Caused undefined behavior. Fixes [#757](https://github.com/metalbear-co/mirrord/issues/757).
 
 ### Misc
 

mirrord/agent/src/file.rs

@@ -29,8 +29,8 @@ pub enum RemoteFile {
 #[derive(Debug, Default)]
 pub struct FileManager {
     root_path: PathBuf,
-    pub open_files: HashMap<usize, RemoteFile>,
-    index_allocator: IndexAllocator<usize>,
+    pub open_files: HashMap<u64, RemoteFile>,
+    index_allocator: IndexAllocator<u64>,
 }
 
 /// Resolve a path that might contain symlinks from a specific container to a path accessible from
@@ -215,7 +215,7 @@ impl FileManager {
     #[tracing::instrument(level = "trace", skip(self))]
     fn open_relative(
         &mut self,
-        relative_fd: usize,
+        relative_fd: u64,
         path: PathBuf,
         open_options: OpenOptionsInternal,
     ) -> RemoteResult<OpenFileResponse> {
@@ -250,17 +250,17 @@ impl FileManager {
     }
 
     #[tracing::instrument(level = "trace", skip(self))]
-    pub(crate) fn read(&mut self, fd: usize, buffer_size: usize) -> RemoteResult<ReadFileResponse> {
+    pub(crate) fn read(&mut self, fd: u64, buffer_size: u64) -> RemoteResult<ReadFileResponse> {
         self.open_files
             .get_mut(&fd)
             .ok_or(ResponseError::NotFound(fd))
             .and_then(|remote_file| {
                 if let RemoteFile::File(file) = remote_file {
-                    let mut buffer = vec![0; buffer_size];
+                    let mut buffer = vec![0; buffer_size as usize];
                     let read_amount =
                         file.read(&mut buffer).map(|read_amount| ReadFileResponse {
                             bytes: buffer,
-                            read_amount,
+                            read_amount: read_amount as u64,
                         })?;
 
                     Ok(read_amount)
@@ -280,27 +280,24 @@ impl FileManager {
     #[tracing::instrument(level = "trace", skip(self))]
     pub(crate) fn read_line(
         &mut self,
-        fd: usize,
-        buffer_size: usize,
+        fd: u64,
+        buffer_size: u64,
     ) -> RemoteResult<ReadFileResponse> {
         self.open_files
             .get_mut(&fd)
             .ok_or(ResponseError::NotFound(fd))
             .and_then(|remote_file| {
                 if let RemoteFile::File(file) = remote_file {
                     let mut reader = BufReader::new(std::io::Read::by_ref(file));
-                    let mut buffer = String::with_capacity(buffer_size);
+                    let mut buffer = String::with_capacity(buffer_size as usize);
                     let read_result = reader
                         .read_line(&mut buffer)
                         .and_then(|read_amount| {
                             // Take the new position to update the file's cursor position later.
                             let position_after_read = reader.stream_position()?;
 
                             // Limit the new position to `buffer_size`.
-                            Ok((
-                                read_amount,
-                                position_after_read.clamp(0, buffer_size as u64),
-                            ))
+                            Ok((read_amount, position_after_read.clamp(0, buffer_size)))
                         })
                         .and_then(|(read_amount, seek_to)| {
                             file.seek(SeekFrom::Start(seek_to))?;
@@ -309,7 +306,7 @@ impl FileManager {
                             // return the full buffer.
                             let response = ReadFileResponse {
                                 bytes: buffer.into_bytes(),
-                                read_amount,
+                                read_amount: read_amount as u64,
                             };
 
                             Ok(response)
@@ -325,23 +322,23 @@ impl FileManager {
     #[tracing::instrument(level = "trace", skip(self))]
     pub(crate) fn read_limited(
         &mut self,
-        fd: usize,
-        buffer_size: usize,
+        fd: u64,
+        buffer_size: u64,
         start_from: u64,
     ) -> RemoteResult<ReadFileResponse> {
         self.open_files
             .get_mut(&fd)
             .ok_or(ResponseError::NotFound(fd))
             .and_then(|remote_file| {
                 if let RemoteFile::File(file) = remote_file {
-                    let mut buffer = vec![0; buffer_size];
+                    let mut buffer = vec![0; buffer_size as usize];
 
                     let read_result = file.read_at(&mut buffer, start_from).map(|read_amount| {
                         // We handle the extra bytes in the `pread` hook, so here we can just
                         // return the full buffer.
                         ReadFileResponse {
                             bytes: buffer,
-                            read_amount,
+                            read_amount: read_amount as u64,
                         }
                     })?;
 
@@ -355,7 +352,7 @@ impl FileManager {
     #[tracing::instrument(level = "trace", skip(self))]
     pub(crate) fn write_limited(
         &mut self,
-        fd: usize,
+        fd: u64,
         start_from: u64,
         buffer: Vec<u8>,
     ) -> RemoteResult<WriteFileResponse> {
@@ -364,9 +361,12 @@ impl FileManager {
             .ok_or(ResponseError::NotFound(fd))
             .and_then(|remote_file| {
                 if let RemoteFile::File(file) = remote_file {
-                    let written_amount = file
-                        .write_at(&buffer, start_from)
-                        .map(|written_amount| WriteFileResponse { written_amount })?;
+                    let written_amount =
+                        file.write_at(&buffer, start_from).map(|written_amount| {
+                            WriteFileResponse {
+                                written_amount: written_amount as u64,
+                            }
+                        })?;
 
                     Ok(written_amount)
                 } else {
@@ -375,11 +375,7 @@ impl FileManager {
             })
     }
 
-    pub(crate) fn seek(
-        &mut self,
-        fd: usize,
-        seek_from: SeekFrom,
-    ) -> RemoteResult<SeekFileResponse> {
+    pub(crate) fn seek(&mut self, fd: u64, seek_from: SeekFrom) -> RemoteResult<SeekFileResponse> {
         trace!(
             "FileManager::seek -> fd {:#?} | seek_from {:#?}",
             fd,
@@ -404,7 +400,7 @@ impl FileManager {
 
     pub(crate) fn write(
         &mut self,
-        fd: usize,
+        fd: u64,
         write_bytes: Vec<u8>,
     ) -> RemoteResult<WriteFileResponse> {
         trace!(
@@ -421,7 +417,7 @@ impl FileManager {
                     let write_result =
                         file.write(&write_bytes)
                             .map(|write_amount| WriteFileResponse {
-                                written_amount: write_amount,
+                                written_amount: write_amount as u64,
                             })?;
 
                     Ok(write_result)
@@ -431,7 +427,7 @@ impl FileManager {
             })
     }
 
-    pub(crate) fn close(&mut self, fd: usize) -> RemoteResult<CloseFileResponse> {
+    pub(crate) fn close(&mut self, fd: u64) -> RemoteResult<CloseFileResponse> {
         trace!("FileManager::close -> fd {:#?}", fd,);
 
         let _file = self
@@ -471,7 +467,7 @@ impl FileManager {
     pub(crate) fn xstat(
         &mut self,
         path: Option<PathBuf>,
-        fd: Option<usize>,
+        fd: Option<u64>,
         follow_symlink: bool,
     ) -> RemoteResult<XstatResponse> {
         let path = match (path, fd) {

mirrord/agent/src/outgoing.rs

@@ -97,7 +97,7 @@ async fn layer_recv(
         }) => {
             let daemon_write = match writers
                 .get_mut(&connection_id)
-                .ok_or(ResponseError::NotFound(connection_id as usize))
+                .ok_or(ResponseError::NotFound(connection_id))
             {
                 Ok(writer) => writer.write_all(&bytes).await.map_err(ResponseError::from),
                 Err(fail) => Err(fail),

mirrord/agent/src/outgoing/udp.rs

@@ -126,7 +126,7 @@ impl UdpOutgoingApi {
             set_namespace(namespace).unwrap();
         }
 
-        let mut allocator = IndexAllocator::default();
+        let mut allocator = IndexAllocator::<ConnectionId>::default();
 
         // TODO: Right now we're manually keeping these 2 maps in sync (aviram suggested using
         // `Weak` for `writers`).
@@ -191,7 +191,7 @@ impl UdpOutgoingApi {
                         }) => {
                             let daemon_write = match writers
                                 .get_mut(&connection_id)
-                                .ok_or(ResponseError::NotFound(connection_id as usize))
+                                .ok_or(ResponseError::NotFound(connection_id))
                             {
                                 Ok((mirror, remote_address)) => mirror
                                     .send((BytesMut::from(bytes.as_slice()), *remote_address))

mirrord/agent/src/util.rs

@@ -138,7 +138,10 @@ where
 
 // TODO(alex): Make this more generic, so we can use `default` everywhere, and
 // delete `new`.
-impl Default for IndexAllocator<usize> {
+impl<T> Default for IndexAllocator<T>
+where
+    T: Default + Num + CheckedAdd + NumCast + Clone,
+{
     fn default() -> Self {
         IndexAllocator::new()
     }

mirrord/layer/src/error.rs

@@ -59,7 +59,7 @@ pub(crate) enum HookError {
     SendErrorHookMessage(#[from] SendError<HookMessage>),
 
     #[error("mirrord-layer: Failed creating local file for `remote_fd` `{0}`!")]
-    LocalFileCreation(usize),
+    LocalFileCreation(u64),
 
     #[cfg(target_os = "macos")]
     #[error("mirrord-layer: SIP patch failed with error `{0}`!")]

mirrord/layer/src/file.rs

@@ -40,7 +40,7 @@ pub(crate) mod hooks;
 pub(crate) mod ops;
 
 type LocalFd = RawFd;
-type RemoteFd = usize;
+type RemoteFd = u64;
 
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
 struct RemoteFile {
@@ -508,31 +508,31 @@ pub struct Open {
 
 #[derive(Debug)]
 pub struct OpenRelative {
-    pub(crate) relative_fd: usize,
+    pub(crate) relative_fd: u64,
     pub(crate) path: PathBuf,
     pub(crate) file_channel_tx: ResponseChannel<OpenFileResponse>,
     pub(crate) open_options: OpenOptionsInternal,
 }
 
 #[derive(Debug)]
 pub struct Read<T> {
-    pub(crate) remote_fd: usize,
-    pub(crate) buffer_size: usize,
+    pub(crate) remote_fd: u64,
+    pub(crate) buffer_size: u64,
     // TODO(alex): Only used for `pread`.
     pub(crate) start_from: u64,
     pub(crate) file_channel_tx: ResponseChannel<T>,
 }
 
 #[derive(Debug)]
 pub struct Seek {
-    pub(crate) remote_fd: usize,
+    pub(crate) remote_fd: u64,
     pub(crate) seek_from: SeekFrom,
     pub(crate) file_channel_tx: ResponseChannel<SeekFileResponse>,
 }
 
 #[derive(Debug)]
 pub struct Write<T> {
-    pub(crate) remote_fd: usize,
+    pub(crate) remote_fd: u64,
     pub(crate) write_bytes: Vec<u8>,
     // Only used for `pwrite`.
     pub(crate) start_from: u64,
@@ -541,7 +541,7 @@ pub struct Write<T> {
 
 #[derive(Debug)]
 pub struct Close {
-    pub(crate) fd: usize,
+    pub(crate) fd: u64,
     pub(crate) file_channel_tx: ResponseChannel<CloseFileResponse>,
 }
 

mirrord/layer/src/file/filter.rs

@@ -85,6 +85,7 @@ fn generate_local_set() -> RegexSet {
         r"^/Users",
         r"^/Library",
         r"^/Applications",
+        r"^/System",
         &format!("^.*{}.*$", current_dir.to_string_lossy()),
         &format!("^.*{}.*$", current_binary.to_string_lossy()),
         "^/$", // root