search

gtk-rs / gtk

DEPRECATED, use https://github.com/gtk-rs/gtk3-rs repository instead!
https://gtk-rs.org/
MIT License
1.24k stars 82 forks source link

Fix an ownership issue #932

Closed hfiguiere closed 4 years ago

hfiguiere commented 4 years ago

Without this I get these valgrind errors (or a crash without valgrind, that cause also gdb to crash)

==1466829== Invalid read of size 8
==1466829==    at 0x52B0189: gtk_widget_dispatch_child_properties_changed (gtkwidget.c:4491)
==1466829==    by 0x1263B9: <T as gtk::subclass::widget::WidgetImplExt>::parent_dispatch_child_properties_changed ()
==1466829==    by 0x123091: gtk::subclass::widget::WidgetImpl::dispatch_child_properties_changed ()
==1466829==    by 0x131A4A: gtk::subclass::widget::widget_dispatch_child_properties_changed ()
==1466829==    by 0x52B28FF: g_object_notify_queue_thaw (gobjectnotifyqueue.c:136)
==1466829==    by 0x52B28FF: gtk_widget_thaw_child_notify (gtkwidget.c:4565)
==1466829==    by 0x5051042: gtk_box_pack (gtkbox.c:1561)
==1466829==    by 0x118673: <O as gtk::auto::box_::BoxExt>::pack_start (box_.rs:539)
==1466829==    by 0x118B5B: widget_test::main ()
==1466829==    by 0x11885F: std::rt::lang_start::{{closure}} ()
==1466829==    by 0x164312: std::panicking::try::do_call (in /home/hub/source/niepce/target/debug/examples/widget-test)
==1466829==    by 0x165B19: __rust_maybe_catch_panic (in /home/hub/source/niepce/target/debug/examples/widget-test)
==1466829==    by 0x164D8C: std::rt::lang_start_internal (in /home/hub/source/niepce/target/debug/examples/widget-test)
==1466829==  Address 0x17c119a0 is 0 bytes inside a block of size 16 free'd
==1466829==    at 0x483AA0C: free (vg_replace_malloc.c:540)
==1466829==    by 0x14B572: alloc::alloc::dealloc ()
==1466829==    by 0x14B3F9: <alloc::alloc::Global as core::alloc::Alloc>::dealloc ()
==1466829==    by 0x143638: alloc::raw_vec::RawVec<T,A>::dealloc_buffer (raw_vec.rs:742)
==1466829==    by 0x146FEE: <alloc::raw_vec::RawVec<T,A> as core::ops::drop::Drop>::drop (raw_vec.rs:751)
==1466829==    by 0x12E96E: core::ptr::real_drop_in_place ()
==1466829==    by 0x12F9D1: core::ptr::real_drop_in_place ()
==1466829==    by 0x126335: <T as gtk::subclass::widget::WidgetImplExt>::parent_dispatch_child_properties_changed ()
==1466829==    by 0x123091: gtk::subclass::widget::WidgetImpl::dispatch_child_properties_changed ()
==1466829==    by 0x131A4A: gtk::subclass::widget::widget_dispatch_child_properties_changed ()
==1466829==    by 0x52B28FF: g_object_notify_queue_thaw (gobjectnotifyqueue.c:136)
==1466829==    by 0x52B28FF: gtk_widget_thaw_child_notify (gtkwidget.c:4565)
==1466829==    by 0x5051042: gtk_box_pack (gtkbox.c:1561)
==1466829==  Block was alloc'd at
==1466829==    at 0x483980B: malloc (vg_replace_malloc.c:309)
==1466829==    by 0x14B50B: alloc::alloc::alloc ()
==1466829==    by 0x14B391: <alloc::alloc::Global as core::alloc::Alloc>::alloc ()
==1466829==    by 0x145107: alloc::raw_vec::RawVec<T,A>::reserve_internal (raw_vec.rs:695)
==1466829==    by 0x146D19: alloc::raw_vec::RawVec<T,A>::reserve (raw_vec.rs:520)
==1466829==    by 0x140DE9: alloc::vec::Vec<T>::reserve ()
==1466829==    by 0x12AB08: <alloc::vec::Vec<T> as alloc::vec::SpecExtend<T,I>>::spec_extend ()
==1466829==    by 0x12AD10: <alloc::vec::Vec<T> as alloc::vec::SpecExtend<T,I>>::from_iter ()
==1466829==    by 0x12AE0C: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter ()
==1466829==    by 0x12FAA4: core::iter::traits::iterator::Iterator::collect ()
==1466829==    by 0x126302: <T as gtk::subclass::widget::WidgetImplExt>::parent_dispatch_child_properties_changed ()
==1466829==    by 0x123091: gtk::subclass::widget::WidgetImpl::dispatch_child_properties_changed ()
==1466829== 
==1466829==
GuillaumeGomez commented 4 years ago

Great catch, thanks a lot!

I guess we'll need a minor release...

cc @EPashkin @sdroege

sdroege commented 4 years ago

Looks good to me.

Theoretically this is wrong because the stashes are freed before the pointers are not used anymore, but for param specs and many other types this is not actually a problem as the stash only contains a pointer to itself. Only for types like strings where an actual conversion has to happen and the converted value needs to be kept around it is mandatory to keep the stash around. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

sdroege commented 4 years ago

@GuillaumeGomez Good to go and a bugfix release indeed, please :)

EPashkin commented 4 years ago

👍