Bug 1974259 - Hold an OwnedHandle in an OverlappedOperation a=dmeehan

This avoids cancelling a potentially closed handle.

Original Revision: https://phabricator.services.mozilla.com/D255191

Differential Revision: https://phabricator.services.mozilla.com/D255385

toolkit/crashreporter/crash_helper_common/src/errors.rs

@@ -38,6 +38,8 @@ pub enum IPCError {
     InvalidSize(#[from] TryFromIntError),
     #[error("Error while parsing a file descriptor string")]
     ParseError,
+    #[error("Failed to duplicate clone handle")]
+    CloneHandleFailed(#[source] std::io::Error),
 }
 
 #[derive(Debug, Error)]

toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs

@@ -200,7 +200,9 @@ impl IPCConnector {
         }
 
         self.overlapped = Some(OverlappedOperation::sched_recv(
-            self.as_raw(),
+            self.handle
+                .try_clone()
+                .map_err(IPCError::CloneHandleFailed)?,
             self.event_raw_handle(),
             messages::HEADER_SIZE,
         )?);
@@ -216,14 +218,24 @@ impl IPCConnector {
     }
 
     pub fn send(&self, buff: &[u8]) -> Result<(), IPCError> {
-        let overlapped =
-            OverlappedOperation::sched_send(self.as_raw(), self.event_raw_handle(), buff.to_vec())?;
+        let overlapped = OverlappedOperation::sched_send(
+            self.handle
+                .try_clone()
+                .map_err(IPCError::CloneHandleFailed)?,
+            self.event_raw_handle(),
+            buff.to_vec(),
+        )?;
         overlapped.complete_send(/* wait */ false)
     }
 
     pub fn recv(&self, expected_size: usize) -> Result<(Vec<u8>, Option<AncillaryData>), IPCError> {
-        let overlapped =
-            OverlappedOperation::sched_recv(self.as_raw(), self.event_raw_handle(), expected_size)?;
+        let overlapped = OverlappedOperation::sched_recv(
+            self.handle
+                .try_clone()
+                .map_err(IPCError::CloneHandleFailed)?,
+            self.event_raw_handle(),
+            expected_size,
+        )?;
         let buffer = overlapped.collect_recv(/* wait */ true)?;
         Ok((buffer, None))
     }

toolkit/crashreporter/crash_helper_common/src/ipc_listener/windows.rs

@@ -53,7 +53,9 @@ impl IPCListener {
 
     pub fn listen(&mut self) -> Result<(), IPCError> {
         self.overlapped = Some(OverlappedOperation::listen(
-            self.handle.as_raw_handle() as HANDLE,
+            self.handle
+                .try_clone()
+                .map_err(IPCError::CloneHandleFailed)?,
             self.event_raw_handle(),
         )?);
         Ok(())

toolkit/crashreporter/crash_helper_common/src/platform/windows.rs

@@ -8,7 +8,7 @@ use crate::{
 };
 use std::{
     mem::zeroed,
-    os::windows::io::{FromRawHandle, OwnedHandle, RawHandle},
+    os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle, RawHandle},
     ptr::{null, null_mut},
 };
 use windows_sys::Win32::{
@@ -93,7 +93,7 @@ fn cancel_overlapped_io(handle: HANDLE, overlapped: &mut OVERLAPPED) -> bool {
 }
 
 pub(crate) struct OverlappedOperation {
-    handle: HANDLE,
+    handle: OwnedHandle,
     overlapped: Option<Box<OVERLAPPED>>,
     buffer: Option<Vec<u8>>,
 }
@@ -104,12 +104,16 @@ enum OverlappedOperationType {
 }
 
 impl OverlappedOperation {
-    pub(crate) fn listen(handle: HANDLE, event: HANDLE) -> Result<OverlappedOperation, IPCError> {
+    pub(crate) fn listen(
+        handle: OwnedHandle,
+        event: HANDLE,
+    ) -> Result<OverlappedOperation, IPCError> {
         let mut overlapped = Self::overlapped_with_event(event)?;
 
         // SAFETY: We guarantee that the handle and OVERLAPPED object are both
         // valid and remain so while used by this function.
-        let res = unsafe { ConnectNamedPipe(handle, overlapped.as_mut()) };
+        let res =
+            unsafe { ConnectNamedPipe(handle.as_raw_handle() as HANDLE, overlapped.as_mut()) };
         let error = get_last_error();
 
         if res != FALSE {
@@ -173,7 +177,7 @@ impl OverlappedOperation {
         // and thus guaranteed to be valid.
         let res = unsafe {
             GetOverlappedResultEx(
-                self.handle,
+                self.handle.as_raw_handle() as HANDLE,
                 overlapped.as_ref(),
                 &mut number_of_bytes_transferred,
                 if wait { IO_TIMEOUT as u32 } else { 0 },
@@ -202,7 +206,7 @@ impl OverlappedOperation {
     }
 
     pub(crate) fn sched_recv(
-        handle: HANDLE,
+        handle: OwnedHandle,
         event: HANDLE,
         expected_size: usize,
     ) -> Result<OverlappedOperation, IPCError> {
@@ -214,7 +218,7 @@ impl OverlappedOperation {
         // duration of the asynchronous operation.
         let res = unsafe {
             ReadFile(
-                handle,
+                handle.as_raw_handle() as HANDLE,
                 buffer.as_mut_ptr(),
                 number_of_bytes_to_read,
                 null_mut(),
@@ -243,7 +247,7 @@ impl OverlappedOperation {
     }
 
     pub(crate) fn sched_send(
-        handle: HANDLE,
+        handle: OwnedHandle,
         event: HANDLE,
         mut buffer: Vec<u8>,
     ) -> Result<OverlappedOperation, IPCError> {
@@ -254,7 +258,7 @@ impl OverlappedOperation {
         // duration of the asynchronous operation.
         let res = unsafe {
             WriteFile(
-                handle,
+                handle.as_raw_handle() as HANDLE,
                 buffer.as_mut_ptr(),
                 number_of_bytes_to_write,
                 null_mut(),
@@ -296,7 +300,7 @@ impl OverlappedOperation {
     }
 
     fn cancel_or_leak(&self, mut overlapped: Box<OVERLAPPED>, buffer: Option<Vec<u8>>) {
-        if !cancel_overlapped_io(self.handle, overlapped.as_mut()) {
+        if !cancel_overlapped_io(self.handle.as_raw_handle() as HANDLE, overlapped.as_mut()) {
             // If we cannot cancel the operation we must leak the
             // associated buffers so that they're available in case it
             // ever completes.