Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS unsafe refactor #65

Closed
wants to merge 3 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 34 additions & 22 deletions src/tz_macos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,37 @@ use core_foundation_sys::string::{
use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName};

pub(crate) fn get_timezone_inner() -> Result<String, crate::GetTimezoneError> {
unsafe { get_timezone().ok_or(crate::GetTimezoneError::OsError) }
get_timezone().ok_or(crate::GetTimezoneError::OsError)
}

#[inline]
unsafe fn get_timezone() -> Option<String> {
fn get_timezone() -> Option<String> {
// The longest name in the IANA time zone database is 25 ASCII characters long.
const MAX_LEN: usize = 32;

// Get system time zone, and borrow its name.
let tz = Dropping::new(CFTimeZoneCopySystem())?;
let name = CFTimeZoneGetName(tz.0);
let tz = Dropping::new(unsafe { CFTimeZoneCopySystem() })?;
let name = unsafe { CFTimeZoneGetName(tz.0) };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+// SAFETY: `Dropping::new()` would have returned `None` if the call to `CFTimeZoneCopySystem()` had failed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to @Kijewski. Let's add safety comments to these unsafe blocks. A safety comment should document how the code is upholding the invariants of the unsafe functions being called.

if name.is_null() {
return None;
}

// 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());
{
// New block prevents any variables escaping this scope.

// If the name is encoded in UTF-8, copy it directly.
let cstr_ptr = unsafe { CFStringGetCStringPtr(name, kCFStringEncodingUTF8) };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+// SAFETY: we tested that `name` is not null, so we know that the result of `CFTimeZoneGetName()` is valid

if !cstr_ptr.is_null() {
// Safety: 1) `cstr_ptr` must contain valid nul terminator. 2)
// `cstr_ptr` must be valid for reads of bytes up to and including
// the null terminator. 3) `cstr_ptr` must remain valid for lifetime
// of `cstr`. We assume #1 and #2 are true based on the result from
// CFStringGetCStringPtr. We ensure #3 by cloning the underlying &str
// to a String.
let cstr = unsafe { std::ffi::CStr::from_ptr(cstr_ptr) };
if let Ok(name) = cstr.to_str() {
return Some(name.to_owned());
}
}
}

Expand All @@ -34,18 +44,20 @@ unsafe fn get_timezone() -> Option<String> {
let mut buf_bytes = 0;
let range = CFRange {
location: 0,
length: CFStringGetLength(name),
length: unsafe { CFStringGetLength(name) },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+// SAFETY: we tested that `name` is not null, so we know that the result of `CFTimeZoneGetName()` is valid

};
if CFStringGetBytes(
name,
range,
kCFStringEncodingUTF8,
b'\0',
false as Boolean,
buf.as_mut_ptr(),
buf.len() as isize,
&mut buf_bytes,
) != range.length
if unsafe {
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
Expand All @@ -70,7 +82,7 @@ impl<T> Drop for Dropping<T> {

impl<T> Dropping<T> {
#[inline]
unsafe fn new(v: *const T) -> Option<Self> {
fn new(v: *const T) -> Option<Self> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method must remain unsafe. You can use only safe functions to construct an invalid pointer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function appears to be similar to NonNull::new which is safe. This function has the null pointer check that matches it.

Even if the pointer given is invalid, it's only dereferencing it which is unsafe.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I can see your argument that Dropping::new could be needed to be marked unsafe because the type will deterrence the pointer on drop.

if v.is_null() {
None
} else {
Expand Down