Merge lp:~psusi/ubuntu/utopic/gparted/fix-crashes into lp:ubuntu/utopic/gparted

[Phillip Susi]

Backport of an upstream fix of a cross thread write after free that causes erratic behavior and crashes.

[Brian Murray (brian-murray)]

I've uploaded this to the utopic proposed queue. Thanks for working on this.

review: Approve

=== added directory '.pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch'
=== added file '.pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/.timestamp'
=== added directory '.pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/include'
=== added file '.pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/include/PipeCapture.h'
--- .pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/include/PipeCapture.h	1970-01-01 00:00:00 +0000
+++ .pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/include/PipeCapture.h	2014-10-31 14:25:19 +0000
@@ -0,0 +1,49 @@
+/* Copyright (C) 2013 Phillip Susi
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GPARTED_PIPECAPTURE_H
+#define GPARTED_PIPECAPTURE_H
+
+#include <glibmm/ustring.h>
+#include <glibmm/main.h>
+#include <glibmm/iochannel.h>
+
+namespace GParted {
+
+// captures output pipe of subprocess into a ustring and emits a signal on eof
+class PipeCapture
+{
+	Glib::ustring &buff;
+	Glib::ustring::size_type linestart ;
+	Glib::ustring::size_type cursor ;
+	Glib::ustring::size_type lineend ;
+	Glib::RefPtr<Glib::IOChannel> channel;
+	guint sourceid;
+	bool OnReadable( Glib::IOCondition condition );
+	static gboolean _OnReadable( GIOChannel *source,
+	                             GIOCondition condition,
+	                             gpointer data );
+public:
+	PipeCapture( int fd, Glib::ustring &buffer );
+	void connect_signal();
+	~PipeCapture();
+	sigc::signal<void> signal_eof;
+	sigc::signal<void> signal_update;
+};
+
+} // namepace GParted
+
+#endif /* GPARTED_PIPECAPTURE_H */

=== added directory '.pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/src'
=== added file '.pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/src/PipeCapture.cc'
--- .pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/src/PipeCapture.cc	1970-01-01 00:00:00 +0000
+++ .pc/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch/src/PipeCapture.cc	2014-10-31 14:25:19 +0000
@@ -0,0 +1,111 @@
+/* Copyright (C) 2013 Phillip Susi
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "../include/PipeCapture.h"
+#include <iostream>
+
+namespace GParted {
+
+PipeCapture::PipeCapture( int fd, Glib::ustring &string ) : buff( string ),
+                                                            linestart( 0 ), cursor( 0 ), lineend( 0 ),
+                                                            sourceid( 0 )
+{
+	// tie fd to string
+	// make channel
+	channel = Glib::IOChannel::create_from_fd( fd );
+}
+
+void PipeCapture::connect_signal()
+{
+	// connect handler to signal input/output
+	sourceid = g_io_add_watch( channel->gobj(),
+	                           GIOCondition(G_IO_IN | G_IO_ERR | G_IO_HUP),
+	                           _OnReadable,
+	                           this );
+}
+
+gboolean PipeCapture::_OnReadable( GIOChannel *source,
+				   GIOCondition condition,
+				   gpointer data )
+{
+	PipeCapture *pc = static_cast<PipeCapture *>(data);
+	gboolean rc = pc->OnReadable( Glib::IOCondition(condition) );
+	if (!rc)
+		pc->sourceid = 0;
+	return rc;
+}
+
+
+bool PipeCapture::OnReadable( Glib::IOCondition condition )
+{
+	//Read from pipe and store in buff.  Provide minimal interpretation so
+	//  programs which use text progress bars are displayed correctly.
+	//  Linestart, cursor and lineend are offsets into buff like this:
+	//      "Previous line\n
+	//       Current line.  Text progress bar: XXXXXXXXXX----------"
+	//       ^                                           ^         ^
+	//       linestart                                   cursor    lineend
+	Glib::ustring str;
+	Glib::IOStatus status = channel->read( str, 512 );
+	if (status == Glib::IO_STATUS_NORMAL)
+	{
+		for( Glib::ustring::iterator s = str.begin(); s != str.end(); s++ )
+		{
+			if( *s == '\b' ) {
+				if ( cursor > linestart )
+					cursor -- ;
+			}
+			else if( *s == '\r' )
+				cursor = linestart ;
+			else if( *s == '\n' ) {
+				cursor = lineend ;
+				buff .append( 1, '\n' ) ;
+				cursor ++ ;
+				linestart = cursor ;
+				lineend = cursor ;
+			}
+			else if (*s == '\x01' || *s == '\x02' )
+				//Skip Ctrl-A and Ctrl-B chars e2fsck uses to bracket the progress bar
+				continue;
+			else {
+				if ( cursor < lineend ) {
+					buff .replace( cursor, 1, 1, *s ) ;
+					cursor ++ ;
+				}
+				else {
+					buff .append( 1, *s ) ;
+					cursor ++ ;
+					lineend = cursor ;
+				}
+			}
+		}
+		signal_update.emit();
+		return true;
+	}
+	if (status != Glib::IO_STATUS_EOF)
+		std::cerr << "Pipe IOChannel read failed" << std::endl;
+	// signal completion
+	signal_eof.emit();
+	return false;
+}
+
+PipeCapture::~PipeCapture()
+{
+	if( sourceid > 0 )
+		g_source_remove( sourceid );
+}
+
+} // namespace GParted

=== modified file '.pc/applied-patches'
--- .pc/applied-patches	2014-02-25 14:38:33 +0000
+++ .pc/applied-patches	2014-10-31 14:25:19 +0000
@@ -1,2 +1,3 @@
 01_fix-desktop.patch
 02_use-pkexec.patch
+0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch

=== modified file 'debian/changelog'
--- debian/changelog	2014-07-21 21:20:18 +0000
+++ debian/changelog	2014-10-31 14:25:19 +0000
@@ -1,3 +1,11 @@
+gparted (0.19.0-1ubuntu1) utopic; urgency=medium
+
+  * 0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch:
+    cherry pick upstream commit fixing a memory corruption error that
+    causes various crashes (LP: #1376051).
+
+ -- Phillip Susi <[e-mail]>  Fri, 24 Oct 2014 15:05:27 -0400
+
 gparted (0.19.0-1build1) utopic; urgency=medium
 
   * Rebuild against libparted2.

=== added file 'debian/patches/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch'
--- debian/patches/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch	2014-10-31 14:25:19 +0000
@@ -0,0 +1,215 @@
+From 0fcfd18061a45f208415971f190818a41cc59f47 Mon Sep 17 00:00:00 2001
+From: Mike Fleetwood <[e-mail]>
+Date: Sat, 5 Jul 2014 09:35:45 +0100
+Subject: [PATCH] Prevent cross thread write after free in _OnReadable()
+ (#731752)
+
+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()
+---
+ include/PipeCapture.h |  1 -
+ src/PipeCapture.cc    | 15 +++++----------
+ 2 files changed, 5 insertions(+), 11 deletions(-)
+
+diff --git a/include/PipeCapture.h b/include/PipeCapture.h
+index e37eba8..d3e2152 100644
+--- a/include/PipeCapture.h
++++ b/include/PipeCapture.h
+@@ -31,7 +31,6 @@ class PipeCapture
+ 	Glib::ustring::size_type cursor ;
+ 	Glib::ustring::size_type lineend ;
+ 	Glib::RefPtr<Glib::IOChannel> channel;
+-	guint sourceid;
+ 	bool OnReadable( Glib::IOCondition condition );
+ 	static gboolean _OnReadable( GIOChannel *source,
+ 	                             GIOCondition condition,
+diff --git a/src/PipeCapture.cc b/src/PipeCapture.cc
+index bbb400e..2b5d5f8 100644
+--- a/src/PipeCapture.cc
++++ b/src/PipeCapture.cc
+@@ -20,8 +20,7 @@
+ namespace GParted {
+ 
+ PipeCapture::PipeCapture( int fd, Glib::ustring &string ) : buff( string ),
+-                                                            linestart( 0 ), cursor( 0 ), lineend( 0 ),
+-                                                            sourceid( 0 )
++                                                            linestart( 0 ), cursor( 0 ), lineend( 0 )
+ {
+ 	// tie fd to string
+ 	// make channel
+@@ -31,10 +30,10 @@ PipeCapture::PipeCapture( int fd, Glib::ustring &string ) : buff( string ),
+ void PipeCapture::connect_signal()
+ {
+ 	// connect handler to signal input/output
+-	sourceid = g_io_add_watch( channel->gobj(),
+-	                           GIOCondition(G_IO_IN | G_IO_ERR | G_IO_HUP),
+-	                           _OnReadable,
+-	                           this );
++	g_io_add_watch( channel->gobj(),
++	                GIOCondition(G_IO_IN | G_IO_ERR | G_IO_HUP),
++	                _OnReadable,
++	                this );
+ }
+ 
+ gboolean PipeCapture::_OnReadable( GIOChannel *source,
+@@ -43,8 +42,6 @@ gboolean PipeCapture::_OnReadable( GIOChannel *source,
+ {
+ 	PipeCapture *pc = static_cast<PipeCapture *>(data);
+ 	gboolean rc = pc->OnReadable( Glib::IOCondition(condition) );
+-	if (!rc)
+-		pc->sourceid = 0;
+ 	return rc;
+ }
+ 
+@@ -104,8 +101,6 @@ bool PipeCapture::OnReadable( Glib::IOCondition condition )
+ 
+ PipeCapture::~PipeCapture()
+ {
+-	if( sourceid > 0 )
+-		g_source_remove( sourceid );
+ }
+ 
+ } // namespace GParted
+-- 
+1.9.1
+

=== modified file 'debian/patches/series'
--- debian/patches/series	2014-02-25 14:38:33 +0000
+++ debian/patches/series	2014-10-31 14:25:19 +0000
@@ -1,3 +1,4 @@
 01_fix-desktop.patch
 02_use-pkexec.patch
 
+0001-Prevent-cross-thread-write-after-free-in-_OnReadable.patch

=== modified file 'include/PipeCapture.h'
--- include/PipeCapture.h	2014-02-25 14:38:33 +0000
+++ include/PipeCapture.h	2014-10-31 14:25:19 +0000
@@ -31,7 +31,6 @@
 	Glib::ustring::size_type cursor ;
 	Glib::ustring::size_type lineend ;
 	Glib::RefPtr<Glib::IOChannel> channel;
-	guint sourceid;
 	bool OnReadable( Glib::IOCondition condition );
 	static gboolean _OnReadable( GIOChannel *source,
 	                             GIOCondition condition,

=== modified file 'src/PipeCapture.cc'
--- src/PipeCapture.cc	2014-06-11 10:50:11 +0000
+++ src/PipeCapture.cc	2014-10-31 14:25:19 +0000
@@ -20,8 +20,7 @@
 namespace GParted {
 
 PipeCapture::PipeCapture( int fd, Glib::ustring &string ) : buff( string ),
-                                                            linestart( 0 ), cursor( 0 ), lineend( 0 ),
-                                                            sourceid( 0 )
+                                                            linestart( 0 ), cursor( 0 ), lineend( 0 )
 {
 	// tie fd to string
 	// make channel
@@ -31,10 +30,10 @@
 void PipeCapture::connect_signal()
 {
 	// connect handler to signal input/output
-	sourceid = g_io_add_watch( channel->gobj(),
-	                           GIOCondition(G_IO_IN | G_IO_ERR | G_IO_HUP),
-	                           _OnReadable,
-	                           this );
+	g_io_add_watch( channel->gobj(),
+	                GIOCondition(G_IO_IN | G_IO_ERR | G_IO_HUP),
+	                _OnReadable,
+	                this );
 }
 
 gboolean PipeCapture::_OnReadable( GIOChannel *source,
@@ -43,8 +42,6 @@
 {
 	PipeCapture *pc = static_cast<PipeCapture *>(data);
 	gboolean rc = pc->OnReadable( Glib::IOCondition(condition) );
-	if (!rc)
-		pc->sourceid = 0;
 	return rc;
 }
 
@@ -104,8 +101,6 @@
 
 PipeCapture::~PipeCapture()
 {
-	if( sourceid > 0 )
-		g_source_remove( sourceid );
 }
 
 } // namespace GParted