ui: Fix use-after-free when new controller mapping is added

When a new controller mapping is added to the controller mapping array
in the xemu config, the array is reallocated, and any existing pointers
become invalid and must be reset.

ui/xemu-controllers.h

@@ -23,8 +23,6 @@
 #include "xemu-input.h"
 #include <SDL.h>
 
-#ifdef __cplusplus
-
 enum class RebindEventResult {
     Ignore,
     Complete,
@@ -71,15 +69,4 @@ public:
     }
 };
 
-extern "C" {
-#endif // __cplusplus
-
-extern int *g_keyboard_scancode_map[25];
-
-GamepadMappings *xemu_settings_load_gamepad_mapping(const char *guid, bool reset_to_default);
-
-#ifdef __cplusplus
-} // extern "C"
-#endif
-
 #endif // XEMU_CONTROLLERS_H

ui/xemu-input.c

@@ -28,7 +28,6 @@
 #include "qemu/timer.h"
 #include "qemu/config-file.h"
 
-#include "xemu-controllers.h"
 #include "xemu-input.h"
 #include "xemu-notifications.h"
 #include "xemu-settings.h"
@@ -166,14 +165,8 @@ static void check_and_reset_in_range(int *btn, int min, int max,
     }
 }
 
-static void xemu_input_bindings_reload_controller_map(ControllerState *con, bool reset_to_default)
+static void xemu_input_bindings_set_in_range(ControllerState *con)
 {
-    assert(con->type == INPUT_DEVICE_SDL_GAMECONTROLLER);
-
-    char guid[35] = { 0 };
-    SDL_JoystickGetGUIDString(con->sdl_joystick_guid, guid, sizeof(guid));
-    con->controller_map = xemu_settings_load_gamepad_mapping(guid, reset_to_default);
-
 #define CHECK_RESET_BUTTON(btn)                                            \
     check_and_reset_in_range(&con->controller_map->controller_mapping.btn, \
                              SDL_CONTROLLER_BUTTON_INVALID,                \
@@ -214,6 +207,38 @@ static void xemu_input_bindings_reload_controller_map(ControllerState *con, bool
 #undef CHECK_RESET_AXIS
 }
 
+static void xemu_input_bindings_reload_map(ControllerState *con)
+{
+    assert(con->type == INPUT_DEVICE_SDL_GAMECONTROLLER);
+
+    char guid[35] = { 0 };
+    SDL_JoystickGetGUIDString(con->sdl_joystick_guid, guid, sizeof(guid));
+    if (!xemu_settings_load_gamepad_mapping(guid, &con->controller_map)) {
+        return;
+    }
+
+    // If this controller did not exist in the mapping array, the config will
+    // have been reallocated. Any gamepad mapping pointers for other controllers
+    // are now invalid, and need to be reloaded.
+    ControllerState *iter, *next;
+    bool is_new_mapping;
+    QTAILQ_FOREACH_SAFE (iter, &available_controllers, entry, next) {
+        if (iter == con || iter->type != INPUT_DEVICE_SDL_GAMECONTROLLER) {
+            continue;
+        }
+
+        memset(guid, 0, sizeof(guid));
+        SDL_JoystickGetGUIDString(iter->sdl_joystick_guid, guid, sizeof(guid));
+
+        is_new_mapping =
+            xemu_settings_load_gamepad_mapping(guid, &iter->controller_map);
+        assert(!is_new_mapping &&
+               "Existing controller GUIDs should exist in the config");
+
+        xemu_input_bindings_set_in_range(iter);
+    }
+}
+
 static const char *get_bound_driver(int port)
 {
     assert(port >= 0 && port <= 3);
@@ -353,9 +378,8 @@ void xemu_input_process_sdl_events(const SDL_Event *event)
         SDL_JoystickGetGUIDString(new_con->sdl_joystick_guid, guid_buf, sizeof(guid_buf));
         DPRINTF("Opened %s (%s)\n", new_con->name, guid_buf);
 
-        xemu_input_bindings_reload_controller_map(new_con, /*reset_to_default=*/false);
-
         QTAILQ_INSERT_TAIL(&available_controllers, new_con, entry);
+        xemu_input_bindings_reload_map(new_con);
 
         // Do not replace binding for a currently bound device. In the case that
         // the same GUID is specified multiple times, on different ports, allow
@@ -916,7 +940,9 @@ int xemu_input_get_test_mode(void)
 void xemu_input_reset_input_mapping(ControllerState *state)
 {
     if (state->type == INPUT_DEVICE_SDL_GAMECONTROLLER) {
-        xemu_input_bindings_reload_controller_map(state, /*reset_to_default=*/true);
+        char guid[35] = { 0 };
+        SDL_JoystickGetGUIDString(state->sdl_joystick_guid, guid, sizeof(guid));
+        xemu_settings_reset_controller_mapping(guid);
     } else if (state->type == INPUT_DEVICE_SDL_KEYBOARD) {
         xemu_settings_reset_keyboard_mapping();
     }

ui/xemu-input.h

@@ -68,12 +68,6 @@ enum controller_state_axis_index {
     CONTROLLER_AXIS__COUNT,
 };
 
-#ifdef __cplusplus
-using GamepadMappings = struct config::input::gamepad_mappings;
-#else
-typedef struct gamepad_mappings GamepadMappings;
-#endif
-
 enum controller_input_device_type {
     INPUT_DEVICE_SDL_KEYBOARD,
     INPUT_DEVICE_SDL_GAMECONTROLLER,
@@ -128,6 +122,8 @@ extern const char *bound_drivers[4];
 extern "C" {
 #endif
 
+extern int *g_keyboard_scancode_map[25];
+
 void xemu_input_init(void);
 void xemu_input_process_sdl_events(const SDL_Event *event); // SDL_CONTROLLERDEVICEADDED, SDL_CONTROLLERDEVICEREMOVED
 void xemu_input_update_controllers(void);

ui/xemu-settings.cc

@@ -261,54 +261,75 @@ void remove_net_nat_forward_ports(unsigned int index)
     cnode->store_to_struct(&g_config);
 }
 
-GamepadMappings *xemu_settings_load_gamepad_mapping(const char *guid, bool reset_to_default)
+bool xemu_settings_load_gamepad_mapping(const char *guid,
+                                        GamepadMappings **mapping)
 {
     unsigned int i;
     unsigned int gamepad_mappings_count = g_config.input.gamepad_mappings_count;
     for (i = 0; i < gamepad_mappings_count; ++i) {
-        auto *mapping = &g_config.input.gamepad_mappings[i];
-        if (strcmp(mapping->gamepad_id, guid) != 0) {
-          continue;
-        }
-
-        if (reset_to_default) {
-          break;
+        *mapping = &g_config.input.gamepad_mappings[i];
+        if (strcmp((*mapping)->gamepad_id, guid) != 0) {
+            continue;
         }
 
         // Migrate global 'allow_vibration' setting to the controller config
         if (!g_config.input.allow_vibration) {
-            mapping->enable_rumble = g_config.input.allow_vibration;
+            (*mapping)->enable_rumble = g_config.input.allow_vibration;
         }
 
-        return mapping;
+        return false;
     }
 
     auto cnode = config_tree.child("input")->child("gamepad_mappings");
     cnode->update_from_struct(&g_config);
     cnode->free_allocations(&g_config);
 
-    CNode *mapping_node;
-    if (reset_to_default && i < gamepad_mappings_count) {
-      mapping_node = &cnode->children[i];
-      mapping_node->reset_to_defaults();
-    } else {
-      cnode->children.push_back(*cnode->array_item_type);
-      mapping_node = &cnode->children.back();
-    }
+    cnode->children.push_back(*cnode->array_item_type);
+    CNode *mapping_node = &cnode->children.back();
 
     mapping_node->child("gamepad_id")->set_string(guid);
 
     cnode->store_to_struct(&g_config);
 
-    auto *mapping = &g_config.input
-      .gamepad_mappings[g_config.input.gamepad_mappings_count - 1];
+    *mapping =
+        &g_config.input
+             .gamepad_mappings[g_config.input.gamepad_mappings_count - 1];
 
     // Migrate global 'allow_vibration' setting to the controller config
     if (!g_config.input.allow_vibration) {
-        mapping->enable_rumble = g_config.input.allow_vibration;
+        (*mapping)->enable_rumble = g_config.input.allow_vibration;
     }
 
-    return mapping;
+    return true;
+}
+
+void xemu_settings_reset_controller_mapping(const char *guid)
+{
+    unsigned int gamepad_mappings_count = g_config.input.gamepad_mappings_count;
+
+    unsigned int i;
+    struct config::input::gamepad_mappings *mapping;
+
+    for (i = 0; i < gamepad_mappings_count; ++i) {
+        mapping = &g_config.input.gamepad_mappings[i];
+        if (strcmp(mapping->gamepad_id, guid) == 0) {
+            break;
+        }
+    }
+
+    if (i == gamepad_mappings_count) {
+        return;
+    }
+
+    CNode *cnode = config_tree.child("input")->child("gamepad_mappings");
+    cnode->update_from_struct(&g_config);
+
+    // Careful not to free the mapping array, as other controllers may be using
+    // it
+    CNode *mapping_node = &cnode->children[i];
+    mapping_node->reset_to_defaults();
+    mapping_node->child("gamepad_id")->set_string(guid);
+    mapping_node->store_to_struct(mapping);
 }
 
 void xemu_settings_reset_keyboard_mapping(void)

ui/xemu-settings.h

@@ -34,6 +34,12 @@ extern "C" {
 
 #include "xemu-config.h"
 
+#ifdef __cplusplus
+using GamepadMappings = struct config::input::gamepad_mappings;
+#else
+typedef struct gamepad_mappings GamepadMappings;
+#endif
+
 extern struct config g_config;
 
 // Override the default config file paths
@@ -67,6 +73,15 @@ static inline void xemu_settings_set_string(const char **str, const char *new_st
 void add_net_nat_forward_ports(int host, int guest, CONFIG_NET_NAT_FORWARD_PORTS_PROTOCOL protocol);
 void remove_net_nat_forward_ports(unsigned int index);
 
+
+// Load gamepad mapping for controller with 'guid', setting the mapping pointer
+// to the config entry. Returns true if the mapping did not previously exist.
+bool xemu_settings_load_gamepad_mapping(const char *guid,
+                                        GamepadMappings **mapping);
+
+// Reset controller mapping to default settings.
+void xemu_settings_reset_controller_mapping(const char *guid);
+
 // Reset keyboard mappings to default settings.
 void xemu_settings_reset_keyboard_mapping(void);
 

ui/xui/main-menu.cc

@@ -358,6 +358,8 @@ void MainMenuInputView::Draw()
     ImGui::SetCursorPos(pos);
 
     if (bound_state) {
+        ImGui::PushID(active);
+
         SectionTitle("Expansion Slots");
         // Begin a 2-column layout to render the expansion slots
         ImGui::PushStyleVar(ImGuiStyleVar_ItemSpacing,
@@ -514,51 +516,53 @@ void MainMenuInputView::Draw()
 
         ImGui::PopStyleVar(); // ItemSpacing
         ImGui::Columns(1);
-    }
 
-    SectionTitle("Mapping");
-    ImVec4 tc = ImGui::GetStyle().Colors[ImGuiCol_Header];
-    tc.w = 0.0f;
-    ImGui::PushStyleColor(ImGuiCol_Header, tc);
+        SectionTitle("Mapping");
+        ImVec4 tc = ImGui::GetStyle().Colors[ImGuiCol_Header];
+        tc.w = 0.0f;
+        ImGui::PushStyleColor(ImGuiCol_Header, tc);
 
-    if (ImGui::CollapsingHeader("Input Mapping")) {
-        float p = ImGui::GetFrameHeight() * 0.3;
-        ImGui::PushStyleVar(ImGuiStyleVar_CellPadding, ImVec2(p, p));
-        if (ImGui::BeginTable("input_remap_tbl", 2,
-                              ImGuiTableFlags_RowBg |
-                                  ImGuiTableFlags_Borders)) {
-            ImGui::TableSetupColumn("Emulated Input");
-            ImGui::TableSetupColumn("Host Input");
-            ImGui::TableHeadersRow();
+        if (ImGui::CollapsingHeader("Input Mapping")) {
+            float p = ImGui::GetFrameHeight() * 0.3;
+            ImGui::PushStyleVar(ImGuiStyleVar_CellPadding, ImVec2(p, p));
+            if (ImGui::BeginTable("input_remap_tbl", 2,
+                                  ImGuiTableFlags_RowBg |
+                                      ImGuiTableFlags_Borders)) {
+                ImGui::TableSetupColumn("Emulated Input");
+                ImGui::TableSetupColumn("Host Input");
+                ImGui::TableHeadersRow();
 
-            PopulateTableController(bound_state);
+                PopulateTableController(bound_state);
 
-            ImGui::EndTable();
+                ImGui::EndTable();
+            }
+            ImGui::PopStyleVar();
         }
-        ImGui::PopStyleVar();
-    }
 
-    if (bound_state && bound_state->type == INPUT_DEVICE_SDL_GAMECONTROLLER) {
-        Toggle("Enable Rumble", &bound_state->controller_map->enable_rumble);
-        Toggle("Invert Left X Axis",
-               &bound_state->controller_map->controller_mapping
-                    .invert_axis_left_x);
-        Toggle("Invert Left Y Axis",
-               &bound_state->controller_map->controller_mapping
-                    .invert_axis_left_y);
-        Toggle("Invert Right X Axis",
-               &bound_state->controller_map->controller_mapping
-                    .invert_axis_right_x);
-        Toggle("Invert Right Y Axis",
-               &bound_state->controller_map->controller_mapping
-                    .invert_axis_right_y);
-    }
+        if (bound_state->type == INPUT_DEVICE_SDL_GAMECONTROLLER) {
+            Toggle("Enable Rumble",
+                   &bound_state->controller_map->enable_rumble);
+            Toggle("Invert Left X Axis",
+                   &bound_state->controller_map->controller_mapping
+                        .invert_axis_left_x);
+            Toggle("Invert Left Y Axis",
+                   &bound_state->controller_map->controller_mapping
+                        .invert_axis_left_y);
+            Toggle("Invert Right X Axis",
+                   &bound_state->controller_map->controller_mapping
+                        .invert_axis_right_x);
+            Toggle("Invert Right Y Axis",
+                   &bound_state->controller_map->controller_mapping
+                        .invert_axis_right_y);
+        }
 
-    if (ImGui::Button("Reset to Default")) {
-      xemu_input_reset_input_mapping(bound_state);
-    }
+        if (ImGui::Button("Reset to Default")) {
+            xemu_input_reset_input_mapping(bound_state);
+        }
 
-    ImGui::PopStyleColor();
+        ImGui::PopStyleColor();
+        ImGui::PopID();
+    }
 
     SectionTitle("Options");
     Toggle("Auto-bind controllers", &g_config.input.auto_bind,