From 704615721fc8b2570ac3e682d5b8a23360d6f0c7 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@gmail.com>
Date: Thu, 27 Sep 2018 14:44:03 +0100
Subject: [PATCH] Prevent segfault when undoing or resetting non-hotkey rows

This is caused by:

* Not checking the hotkey data is not null when performing a
  hotkey action
* Allowing hotkey actions on non-hotkey rows.

This fixes both of these, and adds an assert to warn if someone
does manage to fire a hotkey action on a non-hotkey row (but it
won't crash).

Fixes: lp:1794756
* https://bugs.launchpad.net/kicad/+bug/1794756
---
 common/widgets/widget_hotkey_list.cpp | 44 ++++++++++++++++++++-------
 include/widgets/widget_hotkey_list.h  |  7 +++++
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/common/widgets/widget_hotkey_list.cpp b/common/widgets/widget_hotkey_list.cpp
index 62f78f75fc..cb7f31def7 100644
--- a/common/widgets/widget_hotkey_list.cpp
+++ b/common/widgets/widget_hotkey_list.cpp
@@ -300,6 +300,18 @@ WIDGET_HOTKEY_CLIENT_DATA* WIDGET_HOTKEY_LIST::GetSelHKClientData()
 }
 
 
+WIDGET_HOTKEY_CLIENT_DATA* WIDGET_HOTKEY_LIST::getExpectedHkClientData( wxTreeListItem aItem )
+{
+    const auto hkdata = GetHKClientData( aItem );
+
+    // This probably means a hotkey-only action is being attempted on
+    // a row that is not a hotkey (like a section heading)
+    wxASSERT_MSG( hkdata != nullptr, "No hotkey data found for list item" );
+
+    return hkdata;
+}
+
+
 void WIDGET_HOTKEY_LIST::UpdateFromClientData()
 {
     for( wxTreeListItem i = GetFirstItem(); i.IsOk(); i = GetNextItem( i ) )
@@ -351,13 +363,10 @@ void WIDGET_HOTKEY_LIST::changeHotkey( CHANGED_HOTKEY& aHotkey, long aKey )
 
 void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
 {
-    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( aItem );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = getExpectedHkClientData( aItem );
 
     if( !hkdata )
-    {
-        // Activated item was not a hotkey row
         return;
-    }
 
     wxString    name = GetItemText( aItem, 0 );
     wxString    current_key = GetItemText( aItem, 1 );
@@ -365,7 +374,7 @@ void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
     wxKeyEvent key_event = HK_PROMPT_DIALOG::PromptForKey( GetParent(), name, current_key );
     long key = MapKeypressToKeycode( key_event );
 
-    if( hkdata && key )
+    if( key )
     {
         changeHotkey( hkdata->GetChangedHotkey(), key );
         UpdateFromClientData();
@@ -375,7 +384,10 @@ void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
 
 void WIDGET_HOTKEY_LIST::ResetItem( wxTreeListItem aItem )
 {
-    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( aItem );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = getExpectedHkClientData( aItem );
+
+    if( !hkdata )
+        return;
 
     auto& changed_hk = hkdata->GetChangedHotkey();
     const auto& orig_hk = changed_hk.GetOriginalValue();
@@ -387,7 +399,10 @@ void WIDGET_HOTKEY_LIST::ResetItem( wxTreeListItem aItem )
 
 void WIDGET_HOTKEY_LIST::ResetItemToDefault( wxTreeListItem aItem )
 {
-    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( aItem );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = getExpectedHkClientData( aItem );
+
+    if( !hkdata )
+        return;
 
     auto& changed_hk = hkdata->GetChangedHotkey();
 
@@ -409,10 +424,17 @@ void WIDGET_HOTKEY_LIST::OnContextMenu( wxTreeListEvent& aEvent )
 
     wxMenu menu;
 
-    menu.Append( ID_EDIT, _( "Edit..." ) );
-    menu.Append( ID_RESET, _( "Undo Changes" ) );
-    menu.Append( ID_DEFAULT, _( "Restore Default" ) );
-    menu.Append( wxID_SEPARATOR );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( m_context_menu_item );
+
+    // Some actions only apply if the row is hotkey data
+    if( hkdata )
+    {
+        menu.Append( ID_EDIT, _( "Edit..." ) );
+        menu.Append( ID_RESET, _( "Undo Changes" ) );
+        menu.Append( ID_DEFAULT, _( "Restore Default" ) );
+        menu.Append( wxID_SEPARATOR );
+    }
+
     menu.Append( ID_RESET_ALL, _( "Undo All Changes" ) );
     menu.Append( ID_DEFAULT_ALL, _( "Restore All to Default" ) );
 
diff --git a/include/widgets/widget_hotkey_list.h b/include/widgets/widget_hotkey_list.h
index a0f93a8614..2e728d3c89 100644
--- a/include/widgets/widget_hotkey_list.h
+++ b/include/widgets/widget_hotkey_list.h
@@ -61,6 +61,13 @@ class WIDGET_HOTKEY_LIST : public TWO_COLUMN_TREE_LIST
      */
     WIDGET_HOTKEY_CLIENT_DATA* GetSelHKClientData();
 
+    /**
+     * Get the WIDGET_HOTKEY_CLIENT_DATA form an item and assert if it isn't
+     * found. This is for use when the data not being present indicates an
+     * error.
+     */
+    WIDGET_HOTKEY_CLIENT_DATA* getExpectedHkClientData( wxTreeListItem aItem );
+
     /**
      * Method UpdateFromClientData
      * Refresh the visible text on the widget from the rows' client data objects.