[p2v PATCH 0/4] fix crash in "v2v_version" string lifecycle management
We free the "v2v_version" string with a wrong function, which may cause a crash. The last patch fixes this problem. The first three patches work toward eliminating shadowed identifiers, because shadowing impeded my analysis of "v2v_version"'s life cycle. Laszlo Laszlo Ersek (4): miniexpect: upgrade to miniexpect with PR#1 eliminate shadowing of global variables p2v-c.m4: elicit a stricter set of warnings from gcc ssh.c: fix crash in "v2v_version" lifecycle management gui.c | 48 ++++++++++---------- main.c | 11 +++-- miniexpect/miniexpect.c | 2 +- ssh.c | 18 ++++---- m4/p2v-c.m4 | 2 +- miniexpect/README | 23 ++++++---- 6 files changed, 56 insertions(+), 48 deletions(-)
Incorporate miniexpect with <https://github.com/rwmjones/miniexpect/pull/1> applied, so we can build virt-p2v with "-Wall -Wextra -Wshadow" later. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- miniexpect/miniexpect.c | 2 +- miniexpect/README | 23 ++++++++++++-------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/miniexpect/miniexpect.c b/miniexpect/miniexpect.c index 77d781c0be9f..85335147034b 100644 --- a/miniexpect/miniexpect.c +++ b/miniexpect/miniexpect.c @@ -356,7 +356,7 @@ mexp_expect (mexp_h *h, const mexp_regexp *regexps, if (match_data) ovector = pcre2_get_ovector_pointer (match_data); - if (ovector != NULL && ovector[1] >= 0) + if (ovector != NULL && ovector[1] != ~(PCRE2_SIZE)0) h->next_match = ovector[1]; else h->next_match = -1; diff --git a/miniexpect/README b/miniexpect/README index 828c449d5f33..030d26570f41 100644 --- a/miniexpect/README +++ b/miniexpect/README @@ -1,14 +1,19 @@ -miniexpect is a very simple expect-like library for C. +Miniexpect is a very simple expect-like library -It has a saner interface than libexpect, and doesn't depend on Tcl. -It is also thread safe, const-correct and uses modern C standards. +Expect (https://core.tcl-lang.org/expect/index) is a venerable Tcl +program for automating interactive services, often for automating +logins over telnet, ftp, ssh, etc. -It is standalone, except that it requires the PCRE2 (Perl Compatible -Regular Expressions) library from http://www.pcre.org/. The PCRE2 -dependency is fundamental because we want to offer the most powerful -regular expression syntax to match on, but more importantly because -PCRE2 has a convenient way to detect partial matches which made this -library very simple to implement. +Miniexpect is a C library which has a saner interface than libexpect, +and doesn't depend on Tcl. It is also thread safe, const-correct and +uses modern C standards. + +Miniexpect is standalone, except that it requires the PCRE2 (Perl +Compatible Regular Expressions) library from http://www.pcre.org/. +The PCRE2 dependency is fundamental because we want to offer the most +powerful regular expression syntax to match on, but more importantly +because PCRE2 has a convenient way to detect partial matches which +made miniexpect very simple to implement. License -------
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@redhat.com> --- 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; }
Add "-Wextra" because it's good. Add "-Wshadow" because it's good, but covered by neither "-Wall" or "-Wextra". Exclude warnings about unused parameters (i.e., add "-Wno-unused-parameter"); while normally it would be valuable, we have a whole bunch of GTK callbacks with fixed parameter lists but no actual use for those params in our callback bodies. Littering the code with tens of "__attribute__ ((unused))" decorations is arguably worse than silencing this one warning type. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- m4/p2v-c.m4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/m4/p2v-c.m4 b/m4/p2v-c.m4 index bbf48625ae05..d23e1977b921 100644 --- a/m4/p2v-c.m4 +++ b/m4/p2v-c.m4 @@ -38,7 +38,7 @@ AC_ARG_ENABLE([werror], gcc_warnings=$enableval], [gcc_warnings=no] ) -WARN_CFLAGS="-Wall" +WARN_CFLAGS="-Wall -Wextra -Wshadow -Wno-unused-parameter" AC_SUBST([WARN_CFLAGS]) if test "x$gcc_warnings" = "xyes"; then WERROR_CFLAGS="-Werror"
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:
free(): invalid pointer
and then to call abort(). Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- 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
ACK series... Although if I was going to quibble, GNU code tends to use *_p to indicate a predicate (following the LISP convention [1]), whereas I think your meaning is "private to function", so maybe that's confusing to some. Rich. [1] https://stackoverflow.com/questions/70545047/what-does-suffix-p-mean-in-comm... -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On 09/23/22 11:20, Richard W.M. Jones wrote:
ACK series...
Thanks! Commit range 6f1adeba1aab..aa36551e515f.
Although if I was going to quibble, GNU code tends to use *_p to indicate a predicate (following the LISP convention [1]), whereas I think your meaning is "private to function", so maybe that's confusing to some.
Rich.
[1] https://stackoverflow.com/questions/70545047/what-does-suffix-p-mean-in-comm...
Yes, I certainly remembered "_p" from the jargon file: http://www.catb.org/jargon/html/p-convention.html but that was not enough for me to dismiss an otherwise useful suffix for naming variables. By "_p", I meant "parameter". "private" is a good interpretation as well. Cheers Laszlo
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones