glib: Use TryFrom for GString conversions

jf2048 · jf2048 · commit b2d545a8e015 · 2022-03-13T11:38:37.000-04:00
diff --git a/gio/src/file.rs b/gio/src/file.rs
@@ -237,7 +237,7 @@ impl<O: IsA<File>> 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
@@ -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<GString> for Box<str> {
     }
 }
 
-impl From<String> for GString {
+impl TryFrom<String> for GString {
+    type Error = std::ffi::NulError;
     #[inline]
-    fn from(s: String) -> Self {
+    fn try_from(s: String) -> Result<Self, Self::Error> {
         // 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<Box<str>> for GString {
+impl TryFrom<Box<str>> for GString {
+    type Error = std::ffi::NulError;
     #[inline]
-    fn from(s: Box<str>) -> Self {
+    fn try_from(s: Box<str>) -> Result<Self, Self::Error> {
         // Moves the content of the String
-        s.into_string().into()
+        s.into_string().try_into()
     }
 }
 
@@ -678,50 +678,69 @@ 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<Self, Self::Error> {
         // Allocates with the GLib allocator
         unsafe {
             // No check for valid UTF-8 here
             let copy = ffi::g_malloc(s.len() + 1) as *mut c_char;
             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<Vec<u8>> 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<Vec<u8>> for GString {
+    type Error = GStringError;
     #[inline]
-    fn from(s: Vec<u8>) -> Self {
+    fn try_from(s: Vec<u8>) -> Result<Self, Self::Error> {
         // 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<CString> for GString {
-    #[inline]
-    fn from(s: CString) -> Self {
+impl TryFrom<CString> for GString {
+    type Error = std::str::Utf8Error;
+    fn try_from(s: CString) -> Result<Self, Self::Error> {
         // 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<Self, Self::Error> {
+        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<Self>;
 
     unsafe fn from_value(value: &'a crate::Value) -> Self {
-        Self::from(<&str>::from_value(value))
+        Self::try_from(<&str>::from_value(value)).unwrap()
     }
 }
 
@@ -1097,15 +1116,15 @@ 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<str> = gstring.into();
         assert_eq!(foo.as_ref(), "foo");
     }
 
     #[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<str> = 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<GString, i32> = 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
@@ -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<GStr>
+///         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
@@ -336,18 +336,18 @@ 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);
         } else {
             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
@@ -1152,13 +1152,19 @@ mod tests {
         let v = vec!["123", "456"].to_value();
         assert_eq!(
             v.get::<Vec<GString>>(),
-            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::<Vec<GString>>(),
-            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
@@ -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(
