commit af8df68927a9db0a0eb0392356355e182bc78ab9
parent 121f01a45180816c6f57f157caea101dd94d20d5
Author: Gabriele Svelto <gsvelto@mozilla.com>
Date: Thu, 27 Nov 2025 15:29:51 +0000
Bug 1989686 - Stop duplicating handles when scheduling overlapped operations r=afranchuk
This removes the owned handle we used in overlapped operations to avoid
the crash in bug 1974259. The handle is now replaced with a counted
reference to the source handle.
Differential Revision: https://phabricator.services.mozilla.com/D270528
Diffstat:
3 files changed, 41 insertions(+), 37 deletions(-)
diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs
@@ -14,6 +14,7 @@ use std::{
io::Error,
os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle, OwnedHandle, RawHandle},
ptr::null_mut,
+ rc::Rc,
str::FromStr,
time::{Duration, Instant},
};
@@ -68,7 +69,7 @@ fn extract_buffer_and_handle(buffer: Vec<u8>) -> Result<(Vec<u8>, Option<OwnedHa
pub struct IPCConnector {
/// A connected pipe handle
- handle: OwnedHandle,
+ handle: Rc<OwnedHandle>,
/// A handle to an event which will be used for overlapped I/O on the pipe
event: OwnedHandle,
/// Stores the only pending operation we might have on the pipe
@@ -83,7 +84,7 @@ impl IPCConnector {
let event = create_manual_reset_event()?;
Ok(IPCConnector {
- handle,
+ handle: Rc::new(handle),
event,
overlapped: None,
process: None,
@@ -205,11 +206,11 @@ impl IPCConnector {
}
pub fn into_ancillary(self) -> AncillaryData {
- self.handle
+ Rc::try_unwrap(self.handle).expect("Multiple references to the underlying handle")
}
pub fn into_raw_ancillary(self) -> RawAncillaryData {
- self.handle.into_raw()
+ self.into_ancillary().into_raw()
}
pub fn send_message<T>(&self, message: T) -> Result<(), IPCError>
@@ -252,9 +253,7 @@ impl IPCConnector {
}
self.overlapped = Some(OverlappedOperation::sched_recv(
- self.handle
- .try_clone()
- .map_err(IPCError::CloneHandleFailed)?,
+ &self.handle,
self.event_raw_handle(),
HANDLE_SIZE + messages::HEADER_SIZE,
)?);
@@ -281,21 +280,14 @@ impl IPCConnector {
buffer.extend(handle.to_ne_bytes());
buffer.extend(buff);
- let overlapped = OverlappedOperation::sched_send(
- self.handle
- .try_clone()
- .map_err(IPCError::CloneHandleFailed)?,
- self.event_raw_handle(),
- buffer,
- )?;
+ let overlapped =
+ OverlappedOperation::sched_send(&self.handle, self.event_raw_handle(), buffer)?;
overlapped.complete_send(/* wait */ true)
}
pub fn recv(&self, expected_size: usize) -> Result<(Vec<u8>, Option<AncillaryData>), IPCError> {
let overlapped = OverlappedOperation::sched_recv(
- self.handle
- .try_clone()
- .map_err(IPCError::CloneHandleFailed)?,
+ &self.handle,
self.event_raw_handle(),
HANDLE_SIZE + expected_size,
)?;
diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_listener/windows.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_listener/windows.rs
@@ -12,6 +12,7 @@ use std::{
ffi::{CStr, CString, OsString},
os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle, RawHandle},
ptr::null_mut,
+ rc::Rc,
str::FromStr,
};
use windows_sys::Win32::{
@@ -27,9 +28,13 @@ use windows_sys::Win32::{
};
pub struct IPCListener {
+ /// The name of the pipe this listener will be bound to
server_addr: CString,
- handle: OwnedHandle,
+ /// A named pipe handle listening for incoming connections
+ handle: Rc<OwnedHandle>,
+ /// Stores the only pending operation we might have on the pipe
overlapped: Option<OverlappedOperation>,
+ /// A handle to an event which will be used for overlapped I/O on the pipe
event: OwnedHandle,
}
@@ -40,7 +45,7 @@ impl IPCListener {
Ok(IPCListener {
server_addr,
- handle: pipe,
+ handle: Rc::new(pipe),
overlapped: None,
event,
})
@@ -56,28 +61,34 @@ impl IPCListener {
pub(crate) fn listen(&mut self) -> Result<(), IPCError> {
self.overlapped = Some(OverlappedOperation::listen(
- self.handle
- .try_clone()
- .map_err(IPCError::CloneHandleFailed)?,
+ &self.handle,
self.event_raw_handle(),
)?);
Ok(())
}
pub fn accept(&mut self) -> Result<IPCConnector, IPCError> {
- // We should never call accept() on a listener that wasn't
- // already waiting, so panic in that scenario.
- let overlapped = self.overlapped.take().unwrap();
- overlapped.accept(self.handle.as_raw_handle() as HANDLE)?;
- let new_pipe = create_named_pipe(&self.server_addr, /* first_instance */ false)?;
- let connected_pipe = std::mem::replace(&mut self.handle, new_pipe);
+ let connected_pipe = {
+ // We extract the overlapped operation within this scope so
+ // that it's dropped right away. This ensures that only one
+ // reference to the listener's handle remains.
+ let overlapped = self
+ .overlapped
+ .take()
+ .expect("Accepting a connection without listening first");
+ overlapped.accept(self.handle.as_raw_handle() as HANDLE)?;
+ let new_pipe = create_named_pipe(&self.server_addr, /* first_instance */ false)?;
+ std::mem::replace(&mut self.handle, Rc::new(new_pipe))
+ };
// Once we've accepted a new connection and replaced the listener's
// pipe we need to listen again before we return, so that we're ready
// for the next iteration.
self.listen()?;
- IPCConnector::from_ancillary(connected_pipe)
+ // We can guarantee that there's only one reference to this handle at
+ // this point in time.
+ IPCConnector::from_ancillary(Rc::<OwnedHandle>::try_unwrap(connected_pipe).unwrap())
}
/// Serialize this listener into a string that can be passed on the
@@ -100,7 +111,7 @@ impl IPCListener {
let mut listener = IPCListener {
server_addr,
- handle,
+ handle: Rc::new(handle),
overlapped: None,
event,
};
diff --git a/toolkit/crashreporter/crash_helper_common/src/platform/windows.rs b/toolkit/crashreporter/crash_helper_common/src/platform/windows.rs
@@ -11,6 +11,7 @@ use std::{
mem::zeroed,
os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle, RawHandle},
ptr::{null, null_mut},
+ rc::Rc,
};
use windows_sys::Win32::{
Foundation::{
@@ -96,7 +97,7 @@ fn cancel_overlapped_io(handle: HANDLE, overlapped: &mut OVERLAPPED) -> bool {
}
pub(crate) struct OverlappedOperation {
- handle: OwnedHandle,
+ handle: Rc<OwnedHandle>,
overlapped: Option<Box<OVERLAPPED>>,
buffer: Option<Vec<u8>>,
}
@@ -108,7 +109,7 @@ enum OverlappedOperationType {
impl OverlappedOperation {
pub(crate) fn listen(
- handle: OwnedHandle,
+ handle: &Rc<OwnedHandle>,
event: HANDLE,
) -> Result<OverlappedOperation, IPCError> {
let mut overlapped = Self::overlapped_with_event(event)?;
@@ -134,7 +135,7 @@ impl OverlappedOperation {
}
Ok(OverlappedOperation {
- handle,
+ handle: handle.clone(),
overlapped: Some(overlapped),
buffer: None,
})
@@ -209,7 +210,7 @@ impl OverlappedOperation {
}
pub(crate) fn sched_recv(
- handle: OwnedHandle,
+ handle: &Rc<OwnedHandle>,
event: HANDLE,
expected_size: usize,
) -> Result<OverlappedOperation, IPCError> {
@@ -239,7 +240,7 @@ impl OverlappedOperation {
}
Ok(OverlappedOperation {
- handle,
+ handle: handle.clone(),
overlapped: Some(overlapped),
buffer: Some(buffer),
})
@@ -250,7 +251,7 @@ impl OverlappedOperation {
}
pub(crate) fn sched_send(
- handle: OwnedHandle,
+ handle: &Rc<OwnedHandle>,
event: HANDLE,
mut buffer: Vec<u8>,
) -> Result<OverlappedOperation, IPCError> {
@@ -279,7 +280,7 @@ impl OverlappedOperation {
}
Ok(OverlappedOperation {
- handle,
+ handle: handle.clone(),
overlapped: Some(overlapped),
buffer: Some(buffer),
})
