From f711af2b3f85a72b9d20b8a64a9f6f26e559dcdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Laignel?= Date: Wed, 11 Sep 2024 18:47:36 +0200 Subject: [PATCH] gst: anticipate GQuark to GstIdStr lifetime changes GStreamer fixes a memory leak due to GQuarks by switching to GstIdStr. The consequence is that strings previously backed by a GQuark returned by a function will now get their lifetime bound to that of its owner, while the GQuark version ensured static lifetime. Because some functions return a string with the assumption that they are static and because we can't alter the API for existing versions of the bindings, this MR temporarily forces affected strings as GQuarks, thus gaining static lifetime regardless of the GStreamer version actually being used. For newer versions of the bindings, the API will be fixed and GQuarks will be removed in favor a leakless solution. See: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7432 Part-of: --- gstreamer/src/caps_features.rs | 12 ++++++++++++ gstreamer/src/structure.rs | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/gstreamer/src/caps_features.rs b/gstreamer/src/caps_features.rs index 880028f6c..ac4100a66 100644 --- a/gstreamer/src/caps_features.rs +++ b/gstreamer/src/caps_features.rs @@ -360,6 +360,8 @@ impl CapsFeaturesRef { return None; } + // Safety: we can return a GStr based on the feature here because + // the lifetime of the returned value is constrained by &self. Some(CStr::from_ptr(feature).to_str().unwrap()) } } @@ -511,6 +513,8 @@ impl<'a> Iterator for Iter<'a> { self.idx += 1; + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(CStr::from_ptr(feature).to_str().unwrap()) } } @@ -537,6 +541,8 @@ impl<'a> Iterator for Iter<'a> { let feature = ffi::gst_caps_features_get_nth(self.caps_features.as_ptr(), end as u32); assert!(!feature.is_null()); + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(CStr::from_ptr(feature).to_str().unwrap()) } } @@ -552,6 +558,8 @@ impl<'a> Iterator for Iter<'a> { self.n_features as u32 - 1, ); assert!(!feature.is_null()); + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(CStr::from_ptr(feature).to_str().unwrap()) } } @@ -571,6 +579,8 @@ impl<'a> DoubleEndedIterator for Iter<'a> { ffi::gst_caps_features_get_nth(self.caps_features.as_ptr(), self.n_features as u32); assert!(!feature.is_null()); + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(CStr::from_ptr(feature).to_str().unwrap()) } } @@ -589,6 +599,8 @@ impl<'a> DoubleEndedIterator for Iter<'a> { ); assert!(!feature.is_null()); + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(CStr::from_ptr(feature).to_str().unwrap()) } } diff --git a/gstreamer/src/structure.rs b/gstreamer/src/structure.rs index 8c21ba35b..3379419d4 100644 --- a/gstreamer/src/structure.rs +++ b/gstreamer/src/structure.rs @@ -490,7 +490,9 @@ impl StructureRef { #[doc(alias = "gst_structure_get_name")] pub fn name<'a>(&self) -> &'a str { unsafe { - CStr::from_ptr(ffi::gst_structure_get_name(&self.0)) + let name = ffi::gst_structure_get_name(&self.0); + // Ensure the name is static whatever the GStreamer version being used. + CStr::from_ptr(glib::ffi::g_intern_string(name)) .to_str() .unwrap() } @@ -588,7 +590,12 @@ impl StructureRef { let field_name = ffi::gst_structure_nth_field_name(&self.0, idx); assert!(!field_name.is_null()); - Some(CStr::from_ptr(field_name).to_str().unwrap()) + // Ensure the name is static whatever the GStreamer version being used. + Some( + CStr::from_ptr(glib::ffi::g_intern_string(field_name)) + .to_str() + .unwrap(), + ) } } @@ -902,6 +909,8 @@ impl<'a> Iterator for FieldIterator<'a> { return None; } + // Safety: nth_field_name() ensures static lifetime for the returned string, + // whatever the GStreamer version being used. let field_name = self.structure.nth_field_name(self.idx as u32).unwrap(); self.idx += 1; @@ -922,6 +931,8 @@ impl<'a> DoubleEndedIterator for FieldIterator<'a> { } self.n_fields -= 1; + // Safety: nth_field_name() ensures static lifetime for the returned string, + // whatever the GStreamer version being used. Some(self.structure.nth_field_name(self.n_fields as u32).unwrap()) } } @@ -932,6 +943,8 @@ impl<'a> std::iter::FusedIterator for FieldIterator<'a> {} #[derive(Debug)] pub struct Iter<'a> { + // Safety: FieldIterator ensures static lifetime for the returned Item, + // whatever the GStreamer version being used. iter: FieldIterator<'a>, } @@ -1188,4 +1201,21 @@ mod tests { assert_eq!(format!("{:?}", s), "Structure(test { f1: (gchararray) \"abc\", f2: (gchararray) \"bcd\", f3: (gint) 123, f4: Structure(nested { badger: (gboolean) TRUE }), f5: Array([(gchararray) \"a\", (gchararray) \"b\", (gchararray) \"c\"]), f6: List([(gchararray) \"d\", (gchararray) \"e\", (gchararray) \"f\"]) })"); } + + #[test] + fn test_name_and_field_name_lt() { + crate::init().unwrap(); + + let (name, field_name) = { + let s = Structure::builder("name") + .field("field0", "val0") + .field("field1", "val1") + .build(); + + (s.name(), s.nth_field_name(0).unwrap()) + }; + + assert_eq!(name, "name"); + assert_eq!(field_name, "field0"); + } }