Mirrors
/
gparted
mirror of https://gitlab.gnome.org/GNOME/gparted.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Only allow Undo and Apply after merging operations (#699452) 

It was possible to make GParted crash by adding a label, check or new
UUID operation and then applying the operation before the view of
pending operations had finished fully opening.  The operation would be
successfully applied but GParted would crash afterwards.

The fault was that Add_Operation() still enabled the Undo and Apply
buttons and processed the GTK event loop before merging the list of
pending operations.  Faulty code flow went like this:

    activate_*()
        Add_Operation()
            Add operation to the operations[] vector
            Enable Undo and Apply buttons
            Refresh_Visual()
                Process GTK event loop
                    Process Apply button callback applying operations,
                    refreshing display and clearing operations[] vector
        Merge operations in the operations[] vector
        << Core dump here >>
            Merge_Operations()
                Refresh_Visual()

This faulty code flow came about when merging of operations was added
and it didn't appreciate that the operations[] vector could have been
processed and cleared by Add_Operations() before the merge step.
Relevant commit:

    b10349ae37
    Merge overlapping operations (#438573)

Fragment of code in the label operation case:

  2454  void Win_GParted::activate_label_partition()
  2455  {
  ...
  2472          Add_Operation( operation ) ;
  2473
  2474          // Verify if the two operations can be merged
  2475          for ( unsigned int t = 0 ; t < operations .size() - 1 ; t++ )
  2476          {
  2477              if ( operations[ t ] ->type == OPERATION_LABEL_PARTITION )
  2478              {
  2479                  if ( Merge_Operations( t, operations .size() - 1 ) )
  2480                      break;
  2481              }
  2482          }

Commentary in the crashing label operation case:

  2472  The pending operation was already applied when Add_Operation()
        returned resulting in the operations[] vector being cleared
        setting its size to 0.
  2475  The return type of operations.size() is an unsigned integral, so
        the upper limit of the for loop is t < 0UL - 1.  Assuming a
        32-bit machine that's t < 4294967295.
  2477  operations[] vector is access from out of bounds offset 0
        upwards until unallocated memory is accessed resulting in a core
        dump.

Fix this by not enabling the Undo and Apply buttons and processing the
GTK event loop until after merging of operations has been performed.
Fixed code flow goes like this:

    activate_*()
        Add_Operation()
            Add operation to the operations[] vector
        Merge operations in the operations[] vector
            Merge_Operations()
        show_operationslist()
            Enable Undo and Apply buttons
            Refresh_Visual()
                Process GTK event loop
                    Process Apply button callback applying operations,
                    refreshing display and clearing operations[] vector

Not allowing the operations list to be process until after the merge
step is the be correct ordering.  This also prevents the new operation
from flashing up in the operations list and then immediately
disappearing if merged.  In the case of adding the first operation,
delaying enabling the Undo and Apply buttons is enough as the buttons
were previously disabled preventing the operation being applied before
the merge.  In the case of adding further operations, processing of the
GTK event loop must also be delayed until after the merge to prevent the
operations being applied before the merge.  Although that window of
opportunity would only be microseconds.

Bug #699452 - Crash when applying operations before pending operations
              fully displayed
This commit is contained in:
Mike Fleetwood 2013-05-17 17:01:42 +01:00 committed by Curtis Gedak
parent d9a3f879f9
commit 4cc426c6cf
2 changed files with 39 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
include/Win_GParted.h
View File

@ -65,6 +65,7 @@ private:
void Refresh_Visual();
bool Quit_Check_Operations();
void set_valid_operations() ;
void show_operationslist() ;
//convenience functions
void toggle_item( bool state, int menu_item, int toolbar_item = -1 )

64
src/Win_GParted.cc
View File

@ -714,22 +714,6 @@ void Win_GParted::Add_Operation( Operation * operation, int index )
operations .insert( operations .begin() + index, operation ) ;
else
operations .push_back( operation );
allow_undo_clear_apply( true ) ;
Refresh_Visual();
if ( operations .size() == 1 ) //first operation, open operationslist
open_operationslist() ;
//FIXME: A slight flicker may be introduced by this extra display refresh.
//An extra display refresh seems to prevent the disk area visual disk from
// disappearing when enough operations are added to require a scrollbar
// (about 4 operations with default window size).
// Note that commenting out the code to
// "//make scrollwindow focus on the last operation in the list"
// in HBoxOperations::load_operations() prevents this problem from occurring as well.
// See also Win_GParted::activate_undo().
drawingarea_visualdisk .queue_draw() ;
}
else
{
@ -761,8 +745,6 @@ bool Win_GParted::Merge_Operations( unsigned int first, unsigned int second )
operations[ first ]->create_description() ;
remove_operation( second );
Refresh_Visual();
return true;
}
// Two label change operations on the same partition
@ -775,8 +757,6 @@ bool Win_GParted::Merge_Operations( unsigned int first, unsigned int second )
operations[ first ]->create_description() ;
remove_operation( second );
Refresh_Visual();
return true;
}
// Two change-uuid change operations on the same partition
@ -792,8 +772,6 @@ bool Win_GParted::Merge_Operations( unsigned int first, unsigned int second )
operations[ first ]->create_description() ;
remove_operation( second );
Refresh_Visual();
return true;
}
// Two check operations of the same partition
@ -804,8 +782,6 @@ bool Win_GParted::Merge_Operations( unsigned int first, unsigned int second )
{
remove_operation( second );
Refresh_Visual();
return true;
}
// Two format operations of the same partition
@ -818,8 +794,6 @@ bool Win_GParted::Merge_Operations( unsigned int first, unsigned int second )
operations[ first ]->create_description() ;
remove_operation( second );
Refresh_Visual();
return true;
}
@ -1157,6 +1131,28 @@ void Win_GParted::set_valid_operations()
}
}
void Win_GParted::show_operationslist()
{
//Enable (or disable) Undo and Apply buttons
allow_undo_clear_apply( operations .size() ) ;
//Updates view of operations list and sensitivity of Undo and Apply buttons
Refresh_Visual();
if ( operations .size() == 1 ) //first operation, open operationslist
open_operationslist() ;
//FIXME: A slight flicker may be introduced by this extra display refresh.
//An extra display refresh seems to prevent the disk area visual disk from
// disappearing when enough operations are added to require a scrollbar
// (about 4 operations with default window size).
// Note that commenting out the code to
// "//make scrollwindow focus on the last operation in the list"
// in HBoxOperations::load_operations() prevents this problem from occurring as well.
// See also Win_GParted::activate_undo().
drawingarea_visualdisk .queue_draw() ;
}
void Win_GParted::open_operationslist()
{
if ( ! OPERATIONSLIST_OPEN )
@ -1663,6 +1659,8 @@ void Win_GParted::activate_resize()
}
}
}
show_operationslist() ;
}
void Win_GParted::activate_copy()
@ -1768,6 +1766,8 @@ void Win_GParted::activate_paste()
dialog .run() ;
}
}
show_operationslist() ;
}
void Win_GParted::activate_new()
@ -1801,6 +1801,8 @@ void Win_GParted::activate_new()
operation ->icon = render_icon( Gtk::Stock::NEW, Gtk::ICON_SIZE_MENU );
Add_Operation( operation );
show_operationslist() ;
}
}
}
@ -1908,6 +1910,8 @@ void Win_GParted::activate_delete()
Add_Operation( operation ) ;
}
show_operationslist() ;
}
void Win_GParted::activate_info()
@ -2023,6 +2027,8 @@ void Win_GParted::activate_format( GParted::FILESYSTEM new_fs )
Merge_Operations( operations .size() - 2, operations .size() - 1 );
}
}
show_operationslist() ;
}
void Win_GParted::unmount_partition( bool * succes, Glib::ustring * error )
@ -2449,6 +2455,8 @@ void Win_GParted::activate_check()
break;
}
}
show_operationslist() ;
}
void Win_GParted::activate_label_partition()
@ -2480,6 +2488,8 @@ void Win_GParted::activate_label_partition()
break;
}
}
show_operationslist() ;
}
}
@ -2530,6 +2540,8 @@ void Win_GParted::activate_change_uuid()
break;
}
}
show_operationslist() ;
}
void Win_GParted::activate_undo()