From 0819f928a08d4e4cdc60fee728880749465a921d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Kijewski?= Date: Mon, 15 Aug 2022 01:37:38 +0200 Subject: [PATCH 1/3] Fix potential use after free in MacOS / iOS impl Per Ryan Lopopolo's review: > This bit makes me a bit scared with the `Dropping` type and the `CStr` being dropped while a borrowed `&str` is taken from name. Is the `.map(|name| name.to_owned())` a use after free? > > To be sure, I'd probably restructure all of these combinators to use short circuit return to make sure the `Dropping` and `CStr` wrappers are dropped in the right order. --- src/tz_macos.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index a42c00a..6e831fd 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -4,20 +4,19 @@ use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName}; pub(crate) fn get_timezone_inner() -> Result { unsafe { - Dropping::new(CFTimeZoneCopySystem()) - .and_then(|tz| Dropping::new(CFTimeZoneGetName(tz.0))) - .and_then(|name| { + if let Some(tz) = Dropping::new(CFTimeZoneCopySystem()) { + if let Some(name) = Dropping::new(CFTimeZoneGetName(tz.0)) { let name = CFStringGetCStringPtr(name.0, kCFStringEncodingUTF8); - if name.is_null() { - None - } else { - Some(name) + if !name.is_null() { + let name = std::ffi::CStr::from_ptr(name); + if let Ok(name) = name.to_str() { + return Ok(name.to_owned()); + } } - }) - .and_then(|name| std::ffi::CStr::from_ptr(name).to_str().ok()) - .map(|name| name.to_owned()) - .ok_or(crate::GetTimezoneError::OsError) + } + } } + Err(crate::GetTimezoneError::OsError) } struct Dropping(*const T); From 6bdb9ed1ac2325701dc7b4761b0c0a2d2dc7a9a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Kijewski?= Date: Mon, 15 Aug 2022 07:26:17 +0200 Subject: [PATCH 2/3] Allow non UTF-8 time zones Though it is unlikely that the time zone is stored in an encoding other than UTF-8, it's not much work add to support for this edge case. --- src/tz_macos.rs | 68 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/src/tz_macos.rs b/src/tz_macos.rs index 6e831fd..41ead59 100644 --- a/src/tz_macos.rs +++ b/src/tz_macos.rs @@ -1,22 +1,62 @@ -use core_foundation_sys::base::{CFRelease, CFTypeRef}; -use core_foundation_sys::string::{kCFStringEncodingUTF8, CFStringGetCStringPtr}; +use core_foundation_sys::base::{Boolean, CFRange, CFRelease, CFTypeRef}; +use core_foundation_sys::string::{ + kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength, +}; use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName}; pub(crate) fn get_timezone_inner() -> Result { - unsafe { - if let Some(tz) = Dropping::new(CFTimeZoneCopySystem()) { - if let Some(name) = Dropping::new(CFTimeZoneGetName(tz.0)) { - let name = CFStringGetCStringPtr(name.0, kCFStringEncodingUTF8); - if !name.is_null() { - let name = std::ffi::CStr::from_ptr(name); - if let Ok(name) = name.to_str() { - return Ok(name.to_owned()); - } - } - } + unsafe { get_timezone().ok_or(crate::GetTimezoneError::OsError) } +} + +#[inline] +unsafe fn get_timezone() -> Option { + // 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); + 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()); } } - Err(crate::GetTimezoneError::OsError) + + // 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), + }; + 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 !(1..MAX_LEN as isize).contains(&buf_bytes) { + // 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); From 4331500771b5cce4765d140c7a948aa1b907b137 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Kijewski?= Date: Tue, 16 Aug 2022 05:12:29 +0200 Subject: [PATCH 3/3] Increment patch version --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 8d88aae..12e40be 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "iana-time-zone" description = "get the IANA time zone for the current system" -version = "0.1.44" +version = "0.1.45" authors = ["Andrew Straw "] repository = "https://github.com/strawlab/iana-time-zone" license = "MIT OR Apache-2.0"