From f488d92a7cb8e836042c398385c5654e05c4bde9 Mon Sep 17 00:00:00 2001 From: Matt Schulte Date: Sat, 21 Jan 2023 00:00:11 -0800 Subject: [PATCH 1/3] Fix memory leak in SCNetworkReachability::set_callback In SCNetworkReachability::set_callback, an Arc is created called `callback` and then leaked via 'into_raw`. This leak is never fixed causing a memory leak. The fix is to rely on SCNetworkReachabilitySetCallback to call `retain` once and then `release` once when the reference is released via CFRelease. This allows us to drop the `Arc` in `set_callback` while ensuring the reference count for that `Arc` is >0. Fixes #34 --- .../src/network_reachability.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/system-configuration/src/network_reachability.rs b/system-configuration/src/network_reachability.rs index 2d4d5a2..6347e45 100644 --- a/system-configuration/src/network_reachability.rs +++ b/system-configuration/src/network_reachability.rs @@ -247,13 +247,22 @@ impl SCNetworkReachability { let mut callback_context = SCNetworkReachabilityContext { version: 0, - info: Arc::into_raw(callback) as *mut _, + info: Arc::as_ptr(&callback) as *mut _, retain: Some(NetworkReachabilityCallbackContext::::retain_context), release: Some(NetworkReachabilityCallbackContext::::release_context), copyDescription: Some(NetworkReachabilityCallbackContext::::copy_ctx_description), }; if unsafe { + // The call to SCNetworkReachabilitySetCallback will call the + // `retain` callback which will increment the reference count on + // `callback`. Therefore, although `callback` is dropped when this + // function goes out of scope, the reference count will still be >0. + // + // When `SCNetworkReachability` is dropped, `release` is called + // which will drop the reference count on `callback` to 0. + // + // Assumes the pointer pointed to by the `info` member of `callback_context` is still valid. SCNetworkReachabilitySetCallback( self.0, Some(NetworkReachabilityCallbackContext::::callback), @@ -309,17 +318,15 @@ impl NetworkReachabilityCallbackContext< extern "C" fn release_context(ctx: *const c_void) { unsafe { - let _ = Arc::from_raw(ctx as *mut Self); + Arc::decrement_strong_count(ctx as *mut Self); } } extern "C" fn retain_context(ctx_ptr: *const c_void) -> *const c_void { unsafe { - let ctx_ref: Arc = Arc::from_raw(ctx_ptr as *const Self); - let new_ctx = ctx_ref.clone(); - std::mem::forget(ctx_ref); - Arc::into_raw(new_ctx) as *const c_void + Arc::increment_strong_count(ctx_ptr as *mut Self); } + ctx_ptr } } From f66fc82ae48d91eea14a73ad622090e76df01250 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20L=C3=B6nnhager?= Date: Wed, 27 Dec 2023 17:26:19 +0100 Subject: [PATCH 2/3] Explicitly decrement ref count in set_callback --- .../src/network_reachability.rs | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/system-configuration/src/network_reachability.rs b/system-configuration/src/network_reachability.rs index 6347e45..1a1b91a 100644 --- a/system-configuration/src/network_reachability.rs +++ b/system-configuration/src/network_reachability.rs @@ -247,29 +247,32 @@ impl SCNetworkReachability { let mut callback_context = SCNetworkReachabilityContext { version: 0, - info: Arc::as_ptr(&callback) as *mut _, + info: Arc::into_raw(callback) as *mut _, retain: Some(NetworkReachabilityCallbackContext::::retain_context), release: Some(NetworkReachabilityCallbackContext::::release_context), copyDescription: Some(NetworkReachabilityCallbackContext::::copy_ctx_description), }; - if unsafe { - // The call to SCNetworkReachabilitySetCallback will call the - // `retain` callback which will increment the reference count on - // `callback`. Therefore, although `callback` is dropped when this - // function goes out of scope, the reference count will still be >0. - // - // When `SCNetworkReachability` is dropped, `release` is called - // which will drop the reference count on `callback` to 0. - // - // Assumes the pointer pointed to by the `info` member of `callback_context` is still valid. + let result = unsafe { SCNetworkReachabilitySetCallback( self.0, Some(NetworkReachabilityCallbackContext::::callback), &mut callback_context, ) - } == 0u8 - { + }; + + // The call to SCNetworkReachabilitySetCallback will call the + // `retain` callback which will increment the reference count on + // `callback`. Therefore, although the count is decremented below, + // the reference count will still be >0. + // + // When `SCNetworkReachability` is dropped, `release` is called + // which will drop the reference count on `callback` to 0. + // + // Assumes the pointer pointed to by the `info` member of `callback_context` is still valid. + unsafe { Arc::decrement_strong_count(callback_context.info) }; + + if result == 0u8 { Err(SetCallbackError {}) } else { Ok(()) From 88d5500cd725f0baeae1bc896fce089ce7dd5417 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20L=C3=B6nnhager?= Date: Wed, 27 Dec 2023 17:28:22 +0100 Subject: [PATCH 3/3] Add memory leak fix to changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a72625a..b2fd9c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ Line wrap the file at 100 chars. Th ### Changed - Bump minimum supported Rust version (MSRV) to 1.64.0. +### Fixed +- Fix memory leak in `SCNetworkReachability::set_callback`. + ## [0.5.1] - 2023-05-15 ### Added