From 6c06b7a4846939f4710261617d04920cd30fe0e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Kijewski?= Date: Wed, 28 Sep 2022 10:42:27 +0200 Subject: [PATCH 1/7] Refactor MacOS implementation a lot --- src/tz_macos.rs | 131 +++++++++++++++++++++++++++++++----------------- 1 file changed, 85 insertions(+), 46 deletions(-) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index 1e00846..3b080e5 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -1,80 +1,119 @@ use core_foundation_sys::base::{Boolean, CFRange, CFRelease, CFTypeRef}; use core_foundation_sys::string::{ - kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength, + kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength, CFStringRef, }; -use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName}; +use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName, CFTimeZoneRef}; pub(crate) fn get_timezone_inner() -> Result { - unsafe { get_timezone().ok_or(crate::GetTimezoneError::OsError) } + get_timezone().ok_or(crate::GetTimezoneError::OsError) } #[inline] -unsafe fn get_timezone() -> Option { +fn get_timezone() -> Option { // The longest name in the IANA time zone database is 25 ASCII characters long. const MAX_LEN: usize = 32; + let mut buf = [0; MAX_LEN]; // Get system time zone, and borrow its name. - let tz = Dropping::new(CFTimeZoneCopySystem())?; - let name = CFTimeZoneGetName(tz.0); - if name.is_null() { - return None; - } + let tz = SystemTimeZone::new()?; + let name = tz.name()?; // If the name is encoded in UTF-8, copy it directly. - let cstr = CFStringGetCStringPtr(name, kCFStringEncodingUTF8); - if !cstr.is_null() { - let cstr = std::ffi::CStr::from_ptr(cstr); - if let Ok(name) = cstr.to_str() { - return Some(name.to_owned()); - } - } - - // Otherwise convert the name to UTF-8. - let mut buf = [0; MAX_LEN]; - let mut buf_bytes = 0; - let range = CFRange { - location: 0, - length: CFStringGetLength(name), + let name = if let Some(name) = name.as_utf8() { + name + } else { + // Otherwise convert the name to UTF-8. + name.to_utf8(&mut buf)? }; - if CFStringGetBytes( - name, - range, - kCFStringEncodingUTF8, - b'\0', - false as Boolean, - buf.as_mut_ptr(), - buf.len() as isize, - &mut buf_bytes, - ) != range.length - { - // Could not convert the name. - None - } else if buf_bytes < 1 || buf_bytes >= MAX_LEN as isize { + + if name.len() < 1 || name.len() >= MAX_LEN { // The name should not be empty, or excessively long. None } else { - // Convert the name to a `String`. - let name = core::str::from_utf8(&buf[..buf_bytes as usize]).ok()?; Some(name.to_owned()) } } -struct Dropping(*const T); +struct SystemTimeZone(CFTimeZoneRef); + +struct StringRef<'a> { + string: CFStringRef, + phantom: core::marker::PhantomData<&'a SystemTimeZone>, +} -impl Drop for Dropping { - #[inline] +impl Drop for SystemTimeZone { fn drop(&mut self) { + // SAFETY: `SystemTimeZone` is only ever created with a valid `CFTimeZoneRef`. unsafe { CFRelease(self.0 as CFTypeRef) }; } } -impl Dropping { - #[inline] - unsafe fn new(v: *const T) -> Option { +impl SystemTimeZone { + fn new() -> Option { + // SAFETY: No invariants to uphold. We'll release the pointer when we don't need it anymore. + let v = unsafe { CFTimeZoneCopySystem() }; if v.is_null() { None } else { - Some(Dropping(v)) + Some(SystemTimeZone(v)) + } + } + + fn name(&self) -> Option { + // SAFETY: `SystemTimeZone` is only ever created with a valid `CFTimeZoneRef`. + let string = unsafe { CFTimeZoneGetName(self.0) }; + if string.is_null() { + None + } else { + Some(StringRef { + string, + phantom: core::marker::PhantomData, + }) + } + } +} + +impl<'a> StringRef<'a> { + fn as_utf8(&self) -> Option<&'a str> { + // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. + let v = unsafe { CFStringGetCStringPtr(self.string, kCFStringEncodingUTF8) }; + if !v.is_null() { + // SAFETY: `CFStringGetCStringPtr()` returns NUL-terminated strings. + let v = unsafe { std::ffi::CStr::from_ptr(v) }; + if let Ok(v) = v.to_str() { + return Some(v); + } + } + None + } + + fn to_utf8<'b>(&self, buf: &'b mut [u8]) -> Option<&'b str> { + // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. + let length = unsafe { CFStringGetLength(self.string) }; + + let mut buf_bytes = 0; + let range = CFRange { + location: 0, + length, + }; + + let converted_bytes = unsafe { + // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. + CFStringGetBytes( + self.string, + range, + kCFStringEncodingUTF8, + b'\0', + false as Boolean, + buf.as_mut_ptr(), + buf.len() as isize, + &mut buf_bytes, + ) + }; + if converted_bytes != length || buf_bytes < 0 || buf_bytes as usize > buf.len() { + None + } else { + std::str::from_utf8(&buf[..buf_bytes as usize]).ok() } } } From d1b7f74722b0d6787ef2312ce4d54fd61e337eb8 Mon Sep 17 00:00:00 2001 From: Andrew Straw Date: Thu, 29 Sep 2022 16:10:19 +0200 Subject: [PATCH 2/7] Move newtype wrappers into own modules. --- src/tz_macos.rs | 159 ++++++++++++++++++++++++++---------------------- 1 file changed, 86 insertions(+), 73 deletions(-) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index 3b080e5..b95b767 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -1,9 +1,3 @@ -use core_foundation_sys::base::{Boolean, CFRange, CFRelease, CFTypeRef}; -use core_foundation_sys::string::{ - kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength, CFStringRef, -}; -use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName, CFTimeZoneRef}; - pub(crate) fn get_timezone_inner() -> Result { get_timezone().ok_or(crate::GetTimezoneError::OsError) } @@ -15,7 +9,7 @@ fn get_timezone() -> Option { let mut buf = [0; MAX_LEN]; // Get system time zone, and borrow its name. - let tz = SystemTimeZone::new()?; + let tz = system_time_zone::SystemTimeZone::new()?; let name = tz.name()?; // If the name is encoded in UTF-8, copy it directly. @@ -26,7 +20,7 @@ fn get_timezone() -> Option { name.to_utf8(&mut buf)? }; - if name.len() < 1 || name.len() >= MAX_LEN { + if name.is_empty() || name.len() >= MAX_LEN { // The name should not be empty, or excessively long. None } else { @@ -34,86 +28,105 @@ fn get_timezone() -> Option { } } -struct SystemTimeZone(CFTimeZoneRef); - -struct StringRef<'a> { - string: CFStringRef, - phantom: core::marker::PhantomData<&'a SystemTimeZone>, -} +mod system_time_zone { + //! create a safe wrapper around `CFTimeZoneRef` + use core_foundation_sys::base::{CFRelease, CFTypeRef}; + use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName, CFTimeZoneRef}; -impl Drop for SystemTimeZone { - fn drop(&mut self) { - // SAFETY: `SystemTimeZone` is only ever created with a valid `CFTimeZoneRef`. - unsafe { CFRelease(self.0 as CFTypeRef) }; - } -} + pub(crate) struct SystemTimeZone(CFTimeZoneRef); -impl SystemTimeZone { - fn new() -> Option { - // SAFETY: No invariants to uphold. We'll release the pointer when we don't need it anymore. - let v = unsafe { CFTimeZoneCopySystem() }; - if v.is_null() { - None - } else { - Some(SystemTimeZone(v)) + impl Drop for SystemTimeZone { + fn drop(&mut self) { + // SAFETY: `SystemTimeZone` is only ever created with a valid `CFTimeZoneRef`. + unsafe { CFRelease(self.0 as CFTypeRef) }; } } - fn name(&self) -> Option { - // SAFETY: `SystemTimeZone` is only ever created with a valid `CFTimeZoneRef`. - let string = unsafe { CFTimeZoneGetName(self.0) }; - if string.is_null() { - None - } else { - Some(StringRef { - string, - phantom: core::marker::PhantomData, - }) + impl SystemTimeZone { + pub(crate) fn new() -> Option { + // SAFETY: No invariants to uphold. We'll release the pointer when we don't need it anymore. + let v: CFTimeZoneRef = unsafe { CFTimeZoneCopySystem() }; + if v.is_null() { + None + } else { + Some(SystemTimeZone(v)) + } } - } -} -impl<'a> StringRef<'a> { - fn as_utf8(&self) -> Option<&'a str> { - // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. - let v = unsafe { CFStringGetCStringPtr(self.string, kCFStringEncodingUTF8) }; - if !v.is_null() { - // SAFETY: `CFStringGetCStringPtr()` returns NUL-terminated strings. - let v = unsafe { std::ffi::CStr::from_ptr(v) }; - if let Ok(v) = v.to_str() { - return Some(v); + pub(crate) fn name(&self) -> Option> { + // SAFETY: `SystemTimeZone` is only ever created with a valid `CFTimeZoneRef`. + let string = unsafe { CFTimeZoneGetName(self.0) }; + if string.is_null() { + None + } else { + Some(super::string_ref::StringRef::new(string, self)) } } - None } +} - fn to_utf8<'b>(&self, buf: &'b mut [u8]) -> Option<&'b str> { - // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. - let length = unsafe { CFStringGetLength(self.string) }; +mod string_ref { + //! create safe wrapper around `CFStringRef` - let mut buf_bytes = 0; - let range = CFRange { - location: 0, - length, - }; + use core_foundation_sys::{ + base::{Boolean, CFRange}, + string::{ + kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength, + CFStringRef, + }, + }; + + pub(crate) struct StringRef<'a, T> { + string: CFStringRef, + _parent: &'a T, + } - let converted_bytes = unsafe { + impl<'a, T> StringRef<'a, T> { + pub(crate) fn new(string: CFStringRef, _parent: &'a T) -> Self { + Self { string, _parent } + } + + pub(crate) fn as_utf8(&self) -> Option<&'a str> { // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. - CFStringGetBytes( - self.string, - range, - kCFStringEncodingUTF8, - b'\0', - false as Boolean, - buf.as_mut_ptr(), - buf.len() as isize, - &mut buf_bytes, - ) - }; - if converted_bytes != length || buf_bytes < 0 || buf_bytes as usize > buf.len() { + let v = unsafe { CFStringGetCStringPtr(self.string, kCFStringEncodingUTF8) }; + if !v.is_null() { + // SAFETY: `CFStringGetCStringPtr()` returns NUL-terminated strings. + let v = unsafe { std::ffi::CStr::from_ptr(v) }; + if let Ok(v) = v.to_str() { + return Some(v); + } + } None - } else { - std::str::from_utf8(&buf[..buf_bytes as usize]).ok() + } + + pub(crate) fn to_utf8<'b>(&self, buf: &'b mut [u8]) -> Option<&'b str> { + // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. + let length = unsafe { CFStringGetLength(self.string) }; + + let mut buf_bytes = 0; + let range = CFRange { + location: 0, + length, + }; + + let converted_bytes = unsafe { + // SAFETY: `StringRef` is only ever created with a valid `CFStringRef`. + CFStringGetBytes( + self.string, + range, + kCFStringEncodingUTF8, + b'\0', + false as Boolean, + buf.as_mut_ptr(), + buf.len() as isize, + &mut buf_bytes, + ) + }; + if converted_bytes != length || buf_bytes < 0 || buf_bytes as usize > buf.len() { + None + } else { + std::str::from_utf8(&buf[..buf_bytes as usize]).ok() + } } } } From 55ddac33aca8aa4e286c05d16d9b3478995cb6c5 Mon Sep 17 00:00:00 2001 From: Andrew Straw Date: Thu, 29 Sep 2022 21:16:02 +0200 Subject: [PATCH 3/7] cleanup gnarly conditional --- src/tz_macos.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index b95b767..017087f 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -122,11 +122,13 @@ mod string_ref { &mut buf_bytes, ) }; - if converted_bytes != length || buf_bytes < 0 || buf_bytes as usize > buf.len() { - None - } else { - std::str::from_utf8(&buf[..buf_bytes as usize]).ok() + if converted_bytes != length { + return None; } + use std::convert::TryFrom; + let len = usize::try_from(buf_bytes).ok()?; + let s = buf.get(..len)?; + std::str::from_utf8(s).ok() } } } From 747a13fd838737eecc1a22a71ccb5e9cdc74b49c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Kijewski?= Date: Fri, 30 Sep 2022 10:30:23 +0200 Subject: [PATCH 4/7] Minor reformat --- src/tz_macos.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index 017087f..2953a56 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -30,6 +30,7 @@ fn get_timezone() -> Option { mod system_time_zone { //! create a safe wrapper around `CFTimeZoneRef` + use core_foundation_sys::base::{CFRelease, CFTypeRef}; use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName, CFTimeZoneRef}; @@ -68,12 +69,12 @@ mod system_time_zone { mod string_ref { //! create safe wrapper around `CFStringRef` - use core_foundation_sys::{ - base::{Boolean, CFRange}, - string::{ - kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength, - CFStringRef, - }, + use std::convert::TryInto; + + use core_foundation_sys::base::{Boolean, CFRange}; + use core_foundation_sys::string::{ + kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength, + CFStringRef, }; pub(crate) struct StringRef<'a, T> { @@ -125,8 +126,8 @@ mod string_ref { if converted_bytes != length { return None; } - use std::convert::TryFrom; - let len = usize::try_from(buf_bytes).ok()?; + + let len = buf_bytes.try_into().ok()?; let s = buf.get(..len)?; std::str::from_utf8(s).ok() } From 9fbf7a6d7aadc4840181b72b3c4997099be48008 Mon Sep 17 00:00:00 2001 From: Andrew Straw Date: Thu, 6 Oct 2022 14:16:30 +0200 Subject: [PATCH 5/7] mark function as unsafe --- src/tz_macos.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index 2953a56..897a934 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -60,7 +60,8 @@ mod system_time_zone { if string.is_null() { None } else { - Some(super::string_ref::StringRef::new(string, self)) + // SAFETY: here we ensure that `string` is a valid pointer. + Some(unsafe { super::string_ref::StringRef::new(string, self) }) } } } @@ -83,7 +84,8 @@ mod string_ref { } impl<'a, T> StringRef<'a, T> { - pub(crate) fn new(string: CFStringRef, _parent: &'a T) -> Self { + // SAFETY: `StringRef` must be valid pointer + pub(crate) unsafe fn new(string: CFStringRef, _parent: &'a T) -> Self { Self { string, _parent } } From 770edec2125540da62d1eabe351408b4014e27c3 Mon Sep 17 00:00:00 2001 From: Andrew Straw Date: Thu, 6 Oct 2022 14:34:38 +0200 Subject: [PATCH 6/7] add clarifying comment --- src/tz_macos.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index 897a934..308634c 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -80,6 +80,9 @@ mod string_ref { pub(crate) struct StringRef<'a, T> { string: CFStringRef, + // We exclude mutable access to the parent by taking a reference to the + // parent (rather than, for example, just using a marker to enforce the + // parent's lifetime). _parent: &'a T, } From 8b9c9f0af2b35e8a083a128f539997849241d294 Mon Sep 17 00:00:00 2001 From: Andrew Straw Date: Thu, 6 Oct 2022 14:40:53 +0200 Subject: [PATCH 7/7] add documentation about lifetime --- src/tz_macos.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index 308634c..59b92f2 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -54,6 +54,10 @@ mod system_time_zone { } } + /// Get the time zone name as a [super::string_ref::StringRef]. + /// + /// The lifetime of the `StringRef` is bound to our lifetime. Mutable + /// access is also prevented by taking a reference to `self`. pub(crate) fn name(&self) -> Option> { // SAFETY: `SystemTimeZone` is only ever created with a valid `CFTimeZoneRef`. let string = unsafe { CFTimeZoneGetName(self.0) };