-
-
Notifications
You must be signed in to change notification settings - Fork 24
Fix memory unsafety in FontDescription::set_family_static
#186
Conversation
This commit fixes a memory safety issue in this method. `<str as ToGlibPtr>::to_glib_none` returns a null-terminated string which only lives as long as the `Stash` it returns. This leads to use-after-free because `pango_font_description_set_family_static` expects the given string to outlive this `PangoFontDescription`. Obviously, we can't simply pass the value of `str::as_ptr` because it might not be null-terminated. What we need is `&'static CStr` - `CStr` guarantees the memory representation of the string is null-terminated, and `&'static` guarantees it will remain valid throughout the program's lifetime.
src/font_description.rs
Outdated
use FontDescription; | ||
|
||
impl FontDescription { | ||
pub fn set_family_static(&mut self, family: &'static str) { | ||
pub fn set_family_static(&mut self, family: &'static CStr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is rather annoying to use like that. I'd suggest removing the function completely as it doesn't bring much advantage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's impossible to make it as ergonomic as other methods. I would have no problem with it being removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get rid of it then, if someone wants to call it they can still use the FFI functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yvt do you want to update the PR?
We decided to get rid of this method because it was deemed too unergonomic to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@GuillaumeGomez All green |
Thanks! |
It's illegal to pass the result of
<str as ToGlibPtr>::to_glib_none()
to a*_static
function because it expires at the end of the statement.