[gparted] Prevent cross thread write after free in _OnReadable() (#731752)

[Curtis Gedak <gedakc src gnome org>]

commit 0fcfd18061a45f208415971f190818a41cc59f47
Author: Mike Fleetwood <mike fleetwood googlemail com>
Date:   Sat Jul 5 09:35:45 2014 +0100

    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