From 9e2ca9c38b5358db0f32903db48f771e633809b8 Mon Sep 17 00:00:00 2001 From: Jason Francis Date: Fri, 11 Mar 2022 08:34:54 -0500 Subject: [PATCH] glib: Use TryFrom for GString conversions --- gio/src/file.rs | 2 +- glib/src/gstring.rs | 119 ++++++++++++++++++++--------------- glib/src/log.rs | 4 +- glib/src/utils.rs | 14 ++--- glib/src/value.rs | 10 ++- glib/tests/structured_log.rs | 3 +- 6 files changed, 89 insertions(+), 63 deletions(-) diff --git a/gio/src/file.rs b/gio/src/file.rs index a5324fb88eb6..d9fdf233845c 100644 --- a/gio/src/file.rs +++ b/gio/src/file.rs @@ -237,7 +237,7 @@ impl> FileExtManual for O { + 'static, >, > { - let etag = etag.map(glib::GString::from); + let etag = etag.map(|e| glib::GString::try_from(e).unwrap()); Box::pin(crate::GioFuture::new( self, move |obj, cancellable, send| { diff --git a/glib/src/gstring.rs b/glib/src/gstring.rs index 9323c7ddcc81..2adcb77987f5 100644 --- a/glib/src/gstring.rs +++ b/glib/src/gstring.rs @@ -33,7 +33,7 @@ impl GStr { pub fn from_str_with_nul(s: &str) -> Result<&Self, std::ffi::FromBytesWithNulError> { let bytes = s.as_bytes(); CStr::from_bytes_with_nul(bytes)?; - Ok(unsafe { Self::from_bytes_with_nul_unchecked(bytes) }) + Ok(unsafe { Self::from_utf8_with_nul_unchecked(bytes) }) } // rustdoc-stripper-ignore-next /// Unsafely creates a GLib string wrapper from a byte slice. @@ -42,7 +42,7 @@ impl GStr { /// sanity checks. The provided slice **must** be valid UTF-8, nul-terminated and not contain /// any interior nul bytes. #[inline] - pub const unsafe fn from_bytes_with_nul_unchecked(bytes: &[u8]) -> &Self { + pub const unsafe fn from_utf8_with_nul_unchecked(bytes: &[u8]) -> &Self { debug_assert!(!bytes.is_empty() && bytes[bytes.len() - 1] == 0); mem::transmute(bytes) } @@ -53,7 +53,7 @@ impl GStr { #[inline] pub unsafe fn from_ptr<'a>(ptr: *const c_char) -> &'a Self { let cstr = CStr::from_ptr(ptr); - Self::from_bytes_with_nul_unchecked(cstr.to_bytes_with_nul()) + Self::from_utf8_with_nul_unchecked(cstr.to_bytes_with_nul()) } // rustdoc-stripper-ignore-next /// Converts this GLib string to a byte slice containing the trailing 0 byte. @@ -126,7 +126,7 @@ impl GStr { #[macro_export] macro_rules! gstr { ($s:literal) => { - unsafe { $crate::GStr::from_bytes_with_nul_unchecked($crate::cstr_bytes!($s)) } + unsafe { $crate::GStr::from_utf8_with_nul_unchecked($crate::cstr_bytes!($s)) } }; } @@ -142,7 +142,7 @@ impl<'a> TryFrom<&'a CStr> for &'a GStr { #[inline] fn try_from(s: &'a CStr) -> Result { s.to_str()?; - Ok(unsafe { GStr::from_bytes_with_nul_unchecked(s.to_bytes_with_nul()) }) + Ok(unsafe { GStr::from_utf8_with_nul_unchecked(s.to_bytes_with_nul()) }) } } @@ -270,7 +270,7 @@ unsafe impl<'a> crate::value::FromValue<'a> for &'a GStr { let ptr = gobject_ffi::g_value_get_string(value.to_glib_none().0); let cstr = CStr::from_ptr(ptr); assert!(cstr.to_str().is_ok()); - GStr::from_bytes_with_nul_unchecked(cstr.to_bytes_with_nul()) + GStr::from_utf8_with_nul_unchecked(cstr.to_bytes_with_nul()) } } @@ -388,7 +388,7 @@ impl GString { slice::from_raw_parts(ptr.as_ptr() as *const _, len + 1) }, }; - unsafe { GStr::from_bytes_with_nul_unchecked(bytes) } + unsafe { GStr::from_utf8_with_nul_unchecked(bytes) } } // rustdoc-stripper-ignore-next @@ -426,7 +426,7 @@ impl GString { impl Clone for GString { fn clone(&self) -> GString { - self.as_str().into() + self.as_gstr().to_owned() } } @@ -651,23 +651,23 @@ impl From for Box { } } -impl From for GString { +impl TryFrom for GString { + type Error = std::ffi::NulError; #[inline] - fn from(s: String) -> Self { + fn try_from(s: String) -> Result { // Moves the content of the String - unsafe { - // No check for valid UTF-8 here - let cstr = CString::from_vec_unchecked(s.into_bytes()); - GString(Inner::Native(Some(cstr))) - } + // Check for interior nul bytes + let cstr = CString::new(s.into_bytes())?; + Ok(GString(Inner::Native(Some(cstr)))) } } -impl From> for GString { +impl TryFrom> for GString { + type Error = std::ffi::NulError; #[inline] - fn from(s: Box) -> Self { + fn try_from(s: Box) -> Result { // Moves the content of the String - s.into_string().into() + s.into_string().try_into() } } @@ -678,9 +678,10 @@ impl From<&GStr> for GString { } } -impl From<&str> for GString { +impl TryFrom<&str> for GString { + type Error = std::ffi::FromBytesWithNulError; #[inline] - fn from(s: &str) -> Self { + fn try_from(s: &str) -> Result { // Allocates with the GLib allocator unsafe { // No check for valid UTF-8 here @@ -688,40 +689,58 @@ impl From<&str> for GString { ptr::copy_nonoverlapping(s.as_ptr() as *const c_char, copy, s.len() + 1); ptr::write(copy.add(s.len()), 0); - GString(Inner::Foreign { + // Check for interior nul bytes + let check = std::ffi::CStr::from_bytes_with_nul(std::slice::from_raw_parts( + copy as *const _, + s.len() + 1, + )); + if let Err(err) = check { + ffi::g_free(copy as *mut _); + return Err(err); + } + + Ok(GString(Inner::Foreign { ptr: ptr::NonNull::new_unchecked(copy), len: s.len(), - }) + })) } } } -impl From> for GString { +#[derive(thiserror::Error, Debug)] +pub enum GStringError { + #[error("invalid UTF-8")] + Utf8(#[from] std::str::Utf8Error), + #[error("interior nul bytes")] + Nul(#[from] std::ffi::NulError), +} + +impl TryFrom> for GString { + type Error = GStringError; #[inline] - fn from(s: Vec) -> Self { + fn try_from(s: Vec) -> Result { // Moves the content of the Vec - // Also check if it's valid UTF-8 - let cstring = CString::new(s).expect("CString::new failed"); - cstring.into() + // Also check if it's valid UTF-8 and has no interior nuls + let cstring = CString::new(s)?; + Ok(cstring.try_into()?) } } -impl From for GString { - #[inline] - fn from(s: CString) -> Self { +impl TryFrom for GString { + type Error = std::str::Utf8Error; + fn try_from(s: CString) -> Result { // Moves the content of the CString // Also check if it's valid UTF-8 - assert!(s.to_str().is_ok()); - Self(Inner::Native(Some(s))) + s.to_str()?; + Ok(Self(Inner::Native(Some(s)))) } } -impl From<&CStr> for GString { +impl TryFrom<&CStr> for GString { + type Error = std::str::Utf8Error; #[inline] - fn from(c: &CStr) -> Self { - // Creates a copy with the GLib allocator - // Also check if it's valid UTF-8 - c.to_str().unwrap().into() + fn try_from(c: &CStr) -> Result { + c.to_owned().try_into() } } @@ -772,7 +791,7 @@ impl FromGlibPtrNone<*const u8> for GString { assert!(!ptr.is_null()); let cstr = CStr::from_ptr(ptr as *const _); // Also check if it's valid UTF-8 - cstr.to_str().unwrap().into() + cstr.try_into().unwrap() } } @@ -904,16 +923,16 @@ impl<'a> ToGlibPtr<'a, *mut i8> for GString { impl<'a> FromGlibContainer<*const c_char, *const i8> for GString { unsafe fn from_glib_none_num(ptr: *const i8, num: usize) -> Self { if num == 0 || ptr.is_null() { - return Self::from(""); + return Self::try_from("").unwrap(); } let slice = slice::from_raw_parts(ptr as *const u8, num); // Also check if it's valid UTF-8 - std::str::from_utf8(slice).unwrap().into() + std::str::from_utf8(slice).unwrap().try_into().unwrap() } unsafe fn from_glib_container_num(ptr: *const i8, num: usize) -> Self { if num == 0 || ptr.is_null() { - return Self::from(""); + return Self::try_from("").unwrap(); } // Check if it's valid UTF-8 @@ -928,7 +947,7 @@ impl<'a> FromGlibContainer<*const c_char, *const i8> for GString { unsafe fn from_glib_full_num(ptr: *const i8, num: usize) -> Self { if num == 0 || ptr.is_null() { - return Self::from(""); + return Self::try_from("").unwrap(); } // Check if it's valid UTF-8 @@ -1007,7 +1026,7 @@ unsafe impl<'a> crate::value::FromValue<'a> for GString { type Checker = crate::value::GenericValueTypeOrNoneChecker; unsafe fn from_value(value: &'a crate::Value) -> Self { - Self::from(<&str>::from_value(value)) + Self::try_from(<&str>::from_value(value)).unwrap() } } @@ -1097,7 +1116,7 @@ mod tests { #[test] fn test_gstring_from_str() { - let gstring: GString = "foo".into(); + let gstring: GString = "foo".try_into().unwrap(); assert_eq!(gstring.as_str(), "foo"); let foo: Box = gstring.into(); assert_eq!(foo.as_ref(), "foo"); @@ -1105,7 +1124,7 @@ mod tests { #[test] fn test_string_from_gstring() { - let gstring = GString::from("foo"); + let gstring = GString::try_from("foo").unwrap(); assert_eq!(gstring.as_str(), "foo"); let s = String::from(gstring); assert_eq!(s, "foo"); @@ -1114,7 +1133,7 @@ mod tests { #[test] fn test_gstring_from_cstring() { let cstr = CString::new("foo").unwrap(); - let gstring = GString::from(cstr); + let gstring = GString::try_from(cstr).unwrap(); assert_eq!(gstring.as_str(), "foo"); let foo: Box = gstring.into(); assert_eq!(foo.as_ref(), "foo"); @@ -1123,7 +1142,7 @@ mod tests { #[test] fn test_string_from_gstring_from_cstring() { let cstr = CString::new("foo").unwrap(); - let gstring = GString::from(cstr); + let gstring = GString::try_from(cstr).unwrap(); assert_eq!(gstring.as_str(), "foo"); let s = String::from(gstring); assert_eq!(s, "foo"); @@ -1132,7 +1151,7 @@ mod tests { #[test] fn test_vec_u8_to_gstring() { let v: &[u8] = b"foo"; - let s: GString = Vec::from(v).into(); + let s: GString = Vec::from(v).try_into().unwrap(); assert_eq!(s.as_str(), "foo"); } @@ -1165,11 +1184,11 @@ mod tests { fn test_hashmap() { use std::collections::HashMap; - let gstring = GString::from("foo"); + let gstring = GString::try_from("foo").unwrap(); assert_eq!(gstring.as_str(), "foo"); let mut h: HashMap = HashMap::new(); h.insert(gstring, 42); - let gstring: GString = "foo".into(); + let gstring: GString = "foo".try_into().unwrap(); assert!(h.contains_key(&gstring)); } } diff --git a/glib/src/log.rs b/glib/src/log.rs index 9dfb7d7d2dc5..708793457291 100644 --- a/glib/src/log.rs +++ b/glib/src/log.rs @@ -859,8 +859,8 @@ macro_rules! g_printerr { /// "MY_FIELD2" => "abc {}", "def"; /// // single argument can be a &str or a &[u8] or anything else satsfying AsRef<[u8]> /// "MY_FIELD3" => CString::new("my string").unwrap().to_bytes(); -/// // field names can also be dynamic -/// GString::from("MY_FIELD4") => b"a binary string".to_owned(); +/// // field names can also be dynamic, satisfying AsRef +/// GString::try_from("MY_FIELD4").unwrap() => b"a binary string".to_owned(); /// // the main log message goes in the MESSAGE field /// "MESSAGE" => "test: {} {}", 1, 2, ; /// } diff --git a/glib/src/utils.rs b/glib/src/utils.rs index 65c8f669de9a..99e5f9753419 100644 --- a/glib/src/utils.rs +++ b/glib/src/utils.rs @@ -336,7 +336,7 @@ mod tests { fn test_filename_from_uri() { use crate::GString; use std::path::PathBuf; - let uri: GString = "file:///foo/bar.txt".into(); + let uri: GString = "file:///foo/bar.txt".try_into().unwrap(); if let Ok((filename, hostname)) = crate::filename_from_uri(&uri) { assert_eq!(filename, PathBuf::from(r"/foo/bar.txt")); assert_eq!(hostname, None); @@ -344,10 +344,10 @@ mod tests { unreachable!(); } - let uri: GString = "file://host/foo/bar.txt".into(); + let uri: GString = "file://host/foo/bar.txt".try_into().unwrap(); if let Ok((filename, hostname)) = crate::filename_from_uri(&uri) { assert_eq!(filename, PathBuf::from(r"/foo/bar.txt")); - assert_eq!(hostname, Some(GString::from("host"))); + assert_eq!(hostname, Some(GString::try_from("host").unwrap())); } else { unreachable!(); } @@ -358,19 +358,19 @@ mod tests { use crate::GString; assert_eq!( crate::uri_parse_scheme("foo://bar"), - Some(GString::from("foo")) + Some(GString::try_from("foo").unwrap()) ); assert_eq!(crate::uri_parse_scheme("foo"), None); let escaped = crate::uri_escape_string("&foo", None, true); - assert_eq!(escaped, GString::from("%26foo")); + assert_eq!(escaped, GString::try_from("%26foo").unwrap()); let unescaped = crate::uri_unescape_string(escaped.as_str(), None); - assert_eq!(unescaped, Some(GString::from("&foo"))); + assert_eq!(unescaped, Some(GString::try_from("&foo").unwrap())); assert_eq!( crate::uri_unescape_segment(Some("/foo"), None, None), - Some(GString::from("/foo")) + Some(GString::try_from("/foo").unwrap()) ); assert_eq!(crate::uri_unescape_segment(Some("/foo%"), None, None), None); } diff --git a/glib/src/value.rs b/glib/src/value.rs index 5870c6ed00bf..4755efc6cf9e 100644 --- a/glib/src/value.rs +++ b/glib/src/value.rs @@ -1152,13 +1152,19 @@ mod tests { let v = vec!["123", "456"].to_value(); assert_eq!( v.get::>(), - Ok(vec![GString::from("123"), GString::from("456")]) + Ok(vec![ + GString::try_from("123").unwrap(), + GString::try_from("456").unwrap() + ]) ); let v = vec![String::from("123"), String::from("456")].to_value(); assert_eq!( v.get::>(), - Ok(vec![GString::from("123"), GString::from("456")]) + Ok(vec![ + GString::try_from("123").unwrap(), + GString::try_from("456").unwrap() + ]) ); } diff --git a/glib/tests/structured_log.rs b/glib/tests/structured_log.rs index b49e9ece4891..d2ae160a4a22 100644 --- a/glib/tests/structured_log.rs +++ b/glib/tests/structured_log.rs @@ -35,6 +35,7 @@ fn structured_log() { } ); + let my_meta3 = GString::try_from("MY_META3").unwrap(); log_structured!( None, LogLevel::Message, @@ -43,7 +44,7 @@ fn structured_log() { "MESSAGE" => "formatted with meta: {} {}", 123, 456.0; "MY_META2" => "def{}", "ghi"; "EMPTY" => b""; - GString::from("MY_META3") => b"bstring".to_owned(); + my_meta3 => b"bstring".to_owned(); } ); log_structured_array(