Libguestfs - Sep 2022 - [p2v PATCH 4/4] ssh.c: fix crash in "v2v_version" 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 at 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 at 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 at 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 at 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