Fragment of debugging and valgrind output:
D: tid=2193 main()
...
D: tid=2202 GParted_Core::set_devices_thread()
...
D: tid=2202 Utils::execute_command(command="dumpe2fs -h /dev/sda1", output, error, use_C_locale=1)
D: tid=2202 this=0x13fef4a0 PipeCapture::PipeCapture()
D: tid=2202 this=0x13fef4f0 PipeCapture::PipeCapture()
D: tid=2202 this=0x13fef4a0 PipeCapture::connect_signal()
D: sourceid=77
D: tid=2202 this=0x13fef4f0 PipeCapture::connect_signal()
D: sourceid=78
D: tid=2193 data=0x13fef4a0 PipeCapture::_OnReadable()
D: tid=2193 this=0x13fef4a0 PipeCapture::OnReadable()
D: signal_update.emit()
D: return true
D: tid=2193 data=0x13fef4f0 PipeCapture::_OnReadable()
D: tid=2193 this=0x13fef4f0 PipeCapture::OnReadable()
D: signal_update.emit()
D: return true
D: tid=2193 data=0x13fef4a0 PipeCapture::_OnReadable()
D: tid=2193 this=0x13fef4a0 PipeCapture::OnReadable()
D: signal_update.emit()
D: return true
D: tid=2193 data=0x13fef4f0 PipeCapture::_OnReadable()
D: tid=2193 this=0x13fef4f0 PipeCapture::OnReadable()
D: signal_eof.emit()
D: return false
D: (!rc) &(pc->sourceid)=0x13fef518
D: tid=2193 data=0x13fef4a0 PipeCapture::_OnReadable()
D: tid=2193 this=0x13fef4a0 PipeCapture::OnReadable()
D: signal_update.emit()
D: return true
D: tid=2193 data=0x13fef4a0 PipeCapture::_OnReadable()
D: tid=2193 this=0x13fef4a0 PipeCapture::OnReadable()
D: signal_update.emit()
D: return true
D: tid=2193 data=0x13fef4a0 PipeCapture::_OnReadable()
D: tid=2193 this=0x13fef4a0 PipeCapture::OnReadable()
D: signal_eof.emit()
D: tid=2202 this=0x13fef4f0 PipeCapture::~PipeCapture()
D: sourceid=0
D: tid=2202 this=0x13fef4a0 PipeCapture::~PipeCapture()
D: sourceid=77
D: return false
D: (!rc) &(pc->sourceid)=0x13fef4c8
==2193== Thread 1:
==2193== Invalid write of size 4
==2193== at 0x490580: GParted::PipeCapture::_OnReadable(_GIOChannel*, GIOCondition, void*) (PipeCapture.cc:56)
==2193== by 0x38662492A5: g_main_context_dispatch (gmain.c:3066)
==2193== by 0x3866249627: g_main_context_iterate.isra.24 (gmain.c:3713)
==2193== by 0x3866249A39: g_main_loop_run (gmain.c:3907)
==2193== by 0x3D7FD45C26: gtk_main (gtkmain.c:1257)
==2193== by 0x469743: GParted::GParted_Core::set_devices(std::vector<GParted::Device, std::allocator<GParted::Device> >&) (GParted_Core.cc:155)
==2193== by 0x4A78F1: GParted::Win_GParted::menu_gparted_refresh_devices() (Win_GParted.cc:1259)
==2193== by 0x4A7886: GParted::Win_GParted::on_show() (Win_GParted.cc:1253)
==2193== by 0x3D82B2009C: Gtk::Widget_Class::show_callback(_GtkWidget*) (widget.cc:3855)
==2193== by 0x3867210297: g_closure_invoke (gclosure.c:777)
==2193== by 0x3867221B86: signal_emit_unlocked_R (gsignal.c:3516)
==2193== by 0x386722A0F1: g_signal_emit_valist (gsignal.c:3330)
==2193== Address 0x13fef4c8 is not stack'd, malloc'd or (recently) free'd
==2193==
PipeCapture.cc (with debugging):
46 gboolean PipeCapture::_OnReadable( GIOChannel *source,
47 GIOCondition condition,
48 gpointer data )
49 {
50 std::cout << "D: tid=" << (long int)syscall(SYS_gettid) << " data=" << data << " PipeCapture::_OnReadable()" << std::endl;
51 PipeCapture *pc = static_cast<PipeCapture *>(data);
52 gboolean rc = pc->OnReadable( Glib::IOCondition(condition) );
53 if (!rc)
54 {
55 std::cout << "D: (!rc) &(pc->sourceid)=" << &(pc->sourceid) << std::endl;
56 pc->sourceid = 0;
57 }
58 return rc;
59 }
The use after free across threads only happens when an external program
is being executed from a thread other than the main() thread. This is
because by default glib registered callbacks are run by the glib main
loop, which is only called from the main() thread with Gtk::Main::run().
Event sequence:
tid=2193 tid=2202
main()
...
GParted_Core::set_devices()
Glib::Thread::create(... set_devices_thread ...)
Gtk::Main::run() GParted_Core::set_devices_thread()
...
Utils::execute_command("dumpe2fs ... /dev/sda1" ...)
Glib::spawn_async_with_pipes()
PipeCapture outputcapture(out, output)
outputcapture.connect_signal()
//Glib main loop runs callback
PipeCapture::_OnReadable()
pc->OnReadable()
//output read
signal_update.emit()
return true
...
//Glib main loop runs callback
PipeCapture::_OnReadable()
pc->OnReadable()
//eof reached
[1] signal_eof.emit()
return status.exit_status
[2] PipeCapture::~PipeCapture()
[3] return false
[4] pc->sourceid = 0
What is happening is that the PipeCapture destructor [2] is running in
the set_devices_thread() thread and freeing the object's memory as soon
as signal_eof.emit() [1] has been called. Then signal_eof.emit()
returns back to OnReadable() which then returns false [3] back to the
_OnReadable() callback function which then assigns 0 to sourceid member
variable [4] in the already freed object, detected by valgrind as:
Invalid write of size 4
at ... GParted::PipeCapture::_OnReadable(...) (PipeCapture.cc:56)
This is happening because PipeCapture member variable sourceid is being
saved, in a different thread, just so the _OnReadable() callback can be
removed. However a glib IOChannel callback, type GIOFunc(), returning
false will be automatically removed.
GLib Reference Manual 2.26 / IO Channels
https://developer.gnome.org/glib/2.26/glib-IO-Channels.html#GIOFunc
GIOFunc()
Returns : the function should return FALSE if the event source
should be removed
Therefore fix by just not saving the event sourceid at all, and not
calling g_source_remove() to manually remove the callback, but instead
letting glib automatically remove the callback when it returns false.
Bug #731752 - Write after free cross thread race in
PipeCapture::_OnReadable()
OperationDetail was storing its children in a std::vector. This means they
can be moved around in memory arbitrarily, going through indeterminate
lifetimes. This is generally a bad thing for any non trivial object and
in the case of OperationDetail, it created havoc with the way it maintains
pointers between parent/child objects for signal connections. It will now
keep only pointers to children in a std::vector instead, so their lifetime
can be controlled, fixing various crashes.
Bug 729139 - Refactor OperationDetail to address random behavior
Returning false from the OnReadable callback causes the source to
be destroyed, but PipeCapture::~PipeCapture was destroying it a
second time. Prevent this by zeroing out the sourceid.
Bug 729800 - Prevent GSource double-destroy warning messages
Place the LVM2 member names in a single string, separated by new lines,
so that they can be selected all together by the user. This was not
previously possible when each member name was placed in a separate
widget.
A LVM volume group can contain more than one member and comprise a
multi-line entry. As such set the "Members" field header and value
label pair to top vertical alignment.
As part of the work on bug 652044 - uses deprecated APIs, selectable
vertical alignment was defaulted to ALIGN_CENTER for all labels. The
relevant commits can be viewed in comment 26 of said bug report.
https://bugzilla.gnome.org/show_bug.cgi?id=652044#c26
For multi-line labels a vertical ALIGN_CENTER value is not
consistently aesthetically pleasing. This becomes obvious when a
single-line heading label is paired with a multi-line value label.
To improve the aesthetics, a vertical alignment of ALIGN_TOP is
preferred.
Hence re-add the ability to optionally specify a vertical alignment for
labels. If a yalign value is not specified a default vertical alignment
of ALIGN_CENTER is used.
With the volume group name displayed immediately below the status field,
the vgname is no longer needed in the status field.
Bug 690542 - Partition Information Dialog Warning not readable
When the partition information dialog is resized, the data below the
partition graphic would remain left aligned. Improve the aesthetics by
horizontally centering the data in the window similar to the partition
graphic.
Bug 690542 - Partition Information Dialog Warning not readable
Placing the LVM2 member names in a string list enables the member names
to be selected all at once by a user. This was not possible with
separate label widgets for each LVM2 member name.
Bug 690542 - Partition Information Dialog Warning not readable
Make the dialog resizable, add a vertical scrollbar to the information
and messages section, and set the initial height to ensure the dialog
fits entirely on an 800x600 screen.
A default height is required because some window managers, such as
fluxbox used in GParted Live, only permit resizing the height by using
the bottom corners of the dialog. If the dialog is too large for the
screen then the user would not be able to resize it.
Note that two default initial heights are used in an effort to minimize
the amount of extra whitespace.
Bug 690542 - Partition Information Dialog Warning not readable
On some desktop environments, such as Unity on Ubuntu 13.04, the
partition information dialog can display the used/unused/unallocated
field values overlapping with the "(##%)" percentage value. The problem
is pronounced for values with 4 digits to the left of the decimal (for
example a 1024 MiB partition ). To address this problem, these
percentages are placed in their own column.
Bug 690542 - Partition Information Dialog Warning not readable
Organize the partition information field & value areas into two columns
to minimize the amount of vertical space required.
This is part of a series of changes to enable viewing all partition
information details on a minimum 800x600 display.
Part of Bug 690542 - Partition Information Dialog Warning not readable
This is part of a series of changes to enable viewing all partition
information details on a minimum 800x600 display.
Part of Bug 690542 - Partition Information Dialog Warning not readable
This is part of a series of changes to enable viewing all partition
information details on a minimum 800x600 display.
Part of Bug 690542 - Partition Information Dialog Warning not readable
The code used to unnecessarily destroy and re-create the file system
objects on every scan for file system support tools.
Instead only create the file system objects once and just call each
object's get_filesystem_support() method on each rescan.
Prior to commit:
1f3b11748e
Remove GParted_Core::p_filesystem (#683149)
set_proper_filesystems() used to set GParted_Core::p_filesystem member
variable to one of the FileSystem objects, but that was just treating it
like a local variable. After the commit local variables named
p_filesystem were used where required and set_proper_filesystem() became
a function which did nothing other than call get_filesystem_object().
Now remove set_proper_filesystem() altogether and use
get_filesystem_object() in its place.
Comment is outdated after GParted stopped using ped_partition_is_busy()
for busy partition detection of normal and logical partitions since
commit:
4202992063
Fix false busy detection of unusual case with Linux Software RAID (#712533)
The String::ucompose method is for easy, i18n-friendly composition of
strings with Gtkmm. From past experience, String::ucompose should not
be used to build command lines due to differences in how locales handle
number translation. More specifically, not all locales use number
separators (spaces, commas or periods) and the decimal separator
(period or comma) in the same way.
The problem with using String::ucompose for command lines was
originally encountered and corrected in the following commit:
Fix attempt data rescue fail to open read-only view (#673246)
e494eca1f7
Use e2image features added in e2fsprogs 1.42.9 to move/copy
an ext[234] file system more efficiently by skipping unused blocks.
Fall back to the internal copy algorithm if e2image is not found
or does not support move/copy.
Bug #721516 - Use e2image to move/copy ext[234] filesystems
Abderrahim Kitouni