Shadowing of global variables, especially if it's intentional and/or as wide-spread in a codebase as it is in virt-p2v, is very harmful. Eliminate it by renaming a number of function parameters, appending a "_p" suffix. (The shadowing of "v2v_version" interfered with the analysis of the crash that I originally meant to fix today. The fix is now the last patch in the series.) Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 48 ++++++++++---------- main.c | 11 +++-- ssh.c | 14 +++--- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/gui.c b/gui.c index 0e90235f7dfe..4b98a44fbbe9 100644 --- a/gui.c +++ b/gui.c @@ -680,12 +680,14 @@ connection_next_clicked (GtkWidget *w, gpointer data) /*----------------------------------------------------------------------*/ /* Conversion dialog. */ -static void populate_disks (GtkTreeView *disks_list); -static void populate_removable (GtkTreeView *removable_list); -static void populate_interfaces (GtkTreeView *interfaces_list); +static void populate_disks (GtkTreeView *disks_list_p); +static void populate_removable (GtkTreeView *removable_list_p); +static void populate_interfaces (GtkTreeView *interfaces_list_p); static void toggled (GtkCellRendererToggle *cell, gchar *path_str, gpointer data); static void network_edited_callback (GtkCellRendererToggle *cell, gchar *path_str, gchar *new_text, gpointer data); -static gboolean maybe_identify_click (GtkWidget *interfaces_list, GdkEventButton *event, gpointer data); +static gboolean maybe_identify_click (GtkWidget *interfaces_list_p, + GdkEventButton *event, + gpointer data); static void set_disks_from_ui (struct config *); static void set_removable_from_ui (struct config *); static void set_interfaces_from_ui (struct config *); @@ -1093,7 +1095,7 @@ repopulate_output_combo (struct config *config) * Populate the C<Fixed hard disks> treeview. */ static void -populate_disks (GtkTreeView *disks_list) +populate_disks (GtkTreeView *disks_list_p) { GtkListStore *disks_store; GtkCellRenderer *disks_col_convert, *disks_col_device; @@ -1136,11 +1138,11 @@ populate_disks (GtkTreeView *disks_list) -1); } } - gtk_tree_view_set_model (disks_list, + gtk_tree_view_set_model (disks_list_p, GTK_TREE_MODEL (disks_store)); - gtk_tree_view_set_headers_visible (disks_list, TRUE); + gtk_tree_view_set_headers_visible (disks_list_p, TRUE); disks_col_convert = gtk_cell_renderer_toggle_new (); - gtk_tree_view_insert_column_with_attributes (disks_list, + gtk_tree_view_insert_column_with_attributes (disks_list_p, -1, _("Convert"), disks_col_convert, @@ -1148,7 +1150,7 @@ populate_disks (GtkTreeView *disks_list) NULL); gtk_cell_renderer_set_alignment (disks_col_convert, 0.5, 0.0); disks_col_device = gtk_cell_renderer_text_new (); - gtk_tree_view_insert_column_with_attributes (disks_list, + gtk_tree_view_insert_column_with_attributes (disks_list_p, -1, _("Device"), disks_col_device, @@ -1164,7 +1166,7 @@ populate_disks (GtkTreeView *disks_list) * Populate the C<Removable media> treeview. */ static void -populate_removable (GtkTreeView *removable_list) +populate_removable (GtkTreeView *removable_list_p) { GtkListStore *removable_store; GtkCellRenderer *removable_col_convert, *removable_col_device; @@ -1187,11 +1189,11 @@ populate_removable (GtkTreeView *removable_list) -1); } } - gtk_tree_view_set_model (removable_list, + gtk_tree_view_set_model (removable_list_p, GTK_TREE_MODEL (removable_store)); - gtk_tree_view_set_headers_visible (removable_list, TRUE); + gtk_tree_view_set_headers_visible (removable_list_p, TRUE); removable_col_convert = gtk_cell_renderer_toggle_new (); - gtk_tree_view_insert_column_with_attributes (removable_list, + gtk_tree_view_insert_column_with_attributes (removable_list_p, -1, _("Convert"), removable_col_convert, @@ -1199,7 +1201,7 @@ populate_removable (GtkTreeView *removable_list) NULL); gtk_cell_renderer_set_alignment (removable_col_convert, 0.5, 0.0); removable_col_device = gtk_cell_renderer_text_new (); - gtk_tree_view_insert_column_with_attributes (removable_list, + gtk_tree_view_insert_column_with_attributes (removable_list_p, -1, _("Device"), removable_col_device, @@ -1215,7 +1217,7 @@ populate_removable (GtkTreeView *removable_list) * Populate the C<Network interfaces> treeview. */ static void -populate_interfaces (GtkTreeView *interfaces_list) +populate_interfaces (GtkTreeView *interfaces_list_p) { GtkListStore *interfaces_store; GtkCellRenderer *interfaces_col_convert, *interfaces_col_device, @@ -1257,11 +1259,11 @@ populate_interfaces (GtkTreeView *interfaces_list) -1); } } - gtk_tree_view_set_model (interfaces_list, + gtk_tree_view_set_model (interfaces_list_p, GTK_TREE_MODEL (interfaces_store)); - gtk_tree_view_set_headers_visible (interfaces_list, TRUE); + gtk_tree_view_set_headers_visible (interfaces_list_p, TRUE); interfaces_col_convert = gtk_cell_renderer_toggle_new (); - gtk_tree_view_insert_column_with_attributes (interfaces_list, + gtk_tree_view_insert_column_with_attributes (interfaces_list_p, -1, _("Convert"), interfaces_col_convert, @@ -1269,7 +1271,7 @@ populate_interfaces (GtkTreeView *interfaces_list) NULL); gtk_cell_renderer_set_alignment (interfaces_col_convert, 0.5, 0.0); interfaces_col_device = gtk_cell_renderer_text_new (); - gtk_tree_view_insert_column_with_attributes (interfaces_list, + gtk_tree_view_insert_column_with_attributes (interfaces_list_p, -1, _("Device"), interfaces_col_device, @@ -1277,7 +1279,7 @@ populate_interfaces (GtkTreeView *interfaces_list) NULL); gtk_cell_renderer_set_alignment (interfaces_col_device, 0.0, 0.0); interfaces_col_network = gtk_cell_renderer_text_new (); - gtk_tree_view_insert_column_with_attributes (interfaces_list, + gtk_tree_view_insert_column_with_attributes (interfaces_list_p, -1, _("Connect to virtual network"), interfaces_col_network, @@ -1337,7 +1339,7 @@ network_edited_callback (GtkCellRendererToggle *cell, gchar *path_str, * L<https://en.wikibooks.org/wiki/GTK%2B_By_Example/Tree_View/Events> */ static gboolean -maybe_identify_click (GtkWidget *interfaces_list, GdkEventButton *event, +maybe_identify_click (GtkWidget *interfaces_list_p, GdkEventButton *event, gpointer data) { gboolean ret = FALSE; /* Did we handle this event? */ @@ -1352,14 +1354,14 @@ maybe_identify_click (GtkWidget *interfaces_list, GdkEventButton *event, gdouble event_x, event_y; if (gdk_event_get_coords ((const GdkEvent *) event, &event_x, &event_y) - && gtk_tree_view_get_path_at_pos (GTK_TREE_VIEW (interfaces_list), + && gtk_tree_view_get_path_at_pos (GTK_TREE_VIEW (interfaces_list_p), event_x, event_y, &path, &column, NULL, NULL)) { GList *cols; gint column_index; /* Get column index. */ - cols = gtk_tree_view_get_columns (GTK_TREE_VIEW (interfaces_list)); + cols = gtk_tree_view_get_columns (GTK_TREE_VIEW (interfaces_list_p)); column_index = g_list_index (cols, (gpointer) column); g_list_free (cols); diff --git a/main.c b/main.c index 6c44183b65b3..8a93f9eb402b 100644 --- a/main.c +++ b/main.c @@ -121,12 +121,13 @@ display_short_options (const char *format) } static void -display_long_options (const struct option *long_options) +display_long_options (const struct option *long_options_p) { - while (long_options->name) { - if (STRNEQ (long_options->name, "long-options") && STRNEQ (long_options->name, "short-options")) - printf ("--%s\n", long_options->name); - long_options++; + while (long_options_p->name) { + if (STRNEQ (long_options_p->name, "long-options") && + STRNEQ (long_options_p->name, "short-options")) + printf ("--%s\n", long_options_p->name); + long_options_p++; } exit (EXIT_SUCCESS); } diff --git a/ssh.c b/ssh.c index 3b3e441b84ac..059f0e224cba 100644 --- a/ssh.c +++ b/ssh.c @@ -743,7 +743,7 @@ scp_file (struct config *config, const char *target, const char *local, ...) static void add_input_driver (const char *name); static void add_output_driver (const char *name); -static int compatible_version (const char *v2v_version); +static int compatible_version (const char *v2v_version_p); #if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic ignored "-Wsuggest-attribute=noreturn" /* WTF? */ @@ -1018,15 +1018,15 @@ add_output_driver (const char *name) } static int -compatible_version (const char *v2v_version) +compatible_version (const char *v2v_version_p) { unsigned v2v_major, v2v_minor; /* The major version must always be 1 or 2. */ - if (!STRPREFIX (v2v_version, "1.") && !STRPREFIX (v2v_version, "2.")) { + if (!STRPREFIX (v2v_version_p, "1.") && !STRPREFIX (v2v_version_p, "2.")) { set_ssh_error ("virt-v2v major version is neither 1 nor 2 (\"%s\"), " "this version of virt-p2v is not compatible.", - v2v_version); + v2v_version_p); return 0; } @@ -1035,16 +1035,16 @@ compatible_version (const char *v2v_version) * that we published during development, nor (b) using old virt-v2v. * We should remain compatible with any virt-v2v after 1.28. */ - if (sscanf (v2v_version, "%u.%u", &v2v_major, &v2v_minor) != 2) { + if (sscanf (v2v_version_p, "%u.%u", &v2v_major, &v2v_minor) != 2) { set_ssh_internal_error ("cannot parse virt-v2v version string (\"%s\")", - v2v_version); + v2v_version_p); return 0; } if (v2v_major == 1 && v2v_minor < 28) { set_ssh_error ("virt-v2v version is < 1.28 (\"%s\"), " "you must upgrade to virt-v2v >= 1.28 on " - "the conversion server.", v2v_version); + "the conversion server.", v2v_version_p); return 0; }

We only ever set "v2v_version" to non-NULL with pcre2_substring_get_bynumber(), as suggested by valgrind and then verified by code review. Substrings extracted with this function cannot be released with free(); we must call pcre2_substring_free(). http://www.pcre.org/current/doc/html/pcre2_substring_get_bynumber.html http://www.pcre.org/current/doc/html/pcre2_substring_free.html The symptom of this bug is that the second click on the "Test Connection" causes free() to print: and then to call abort(). Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- ssh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ssh.c b/ssh.c index 059f0e224cba..566121b172c5 100644 --- a/ssh.c +++ b/ssh.c @@ -765,7 +765,7 @@ test_connection (struct config *config) /* Clear any previous version information since we may be connecting * to a different server. */ - free (v2v_version); + pcre2_substring_free ((PCRE2_UCHAR *)v2v_version); v2v_version = NULL; /* Send 'virt-v2v --version' command and hope we get back a version string. @@ -788,7 +788,7 @@ test_connection (struct config *config) { 0 } }, match_data)) { case 100: /* Got version string. */ - free (v2v_version); + pcre2_substring_free ((PCRE2_UCHAR *)v2v_version); pcre2_substring_get_bynumber (match_data, 1, (PCRE2_UCHAR **) &v2v_version, &verlen); #if DEBUG_STDERR