Libguestfs - Jan 2017 - Re: [PATCH] lib: Use a common function to validate strings.

As discussed in the thread on validating $TERM, it would be good to
have a common function to do this.  This is such a function.

The main advantage is it includes unit tests which the previous
functions did not.

Rich.

---
 python/Makefile.am              |  5 ++++
 src/appliance-kcmdline.c        | 21 ++-----------
 src/drives.c                    | 65 +++++++----------------------------------
 src/guestfs-internal-frontend.h |  3 ++
 src/unit-tests.c                | 40 +++++++++++++++++++++++++
 src/utils.c                     | 50 +++++++++++++++++++++++++++++--
 6 files changed, 110 insertions(+), 74 deletions(-)

diff --git a/python/Makefile.am b/python/Makefile.am
index 486b52e..10fa0d3 100644
--- a/python/Makefile.am
+++ b/python/Makefile.am
@@ -95,6 +95,7 @@ setup-install: setup.py stamp-extra-files
 # Python's crappy MANIFEST file cannot graft single files, so we have
 # to hard-link any extra files we need into the local directory.
 stamp-extra-files: \
+	  c-ctype.h \
 	  config.h \
 	  guestfs-internal-all.h \
 	  guestfs-internal-frontend-cleanups.h \
@@ -106,6 +107,9 @@ stamp-extra-files: \
 config.h:
 	ln ../config.h $@
 
+c-ctype.h:
+	ln $(top_srcdir)/gnulib/lib/c-ctype.h $@
+
 ignore-value.h:
 	ln $(top_srcdir)/gnulib/lib/ignore-value.h $@
 
@@ -138,6 +142,7 @@ CLEANFILES += \
 	*.pyc \
 	examples/*~ examples/*.pyc \
 	t/*~ t/*.pyc \
+	c-ctype.h \
 	config.h \
 	guestfs-internal-all.h \
 	guestfs-internal-frontend-cleanups.h \
diff --git a/src/appliance-kcmdline.c b/src/appliance-kcmdline.c
index 6771668..c5bc65f 100644
--- a/src/appliance-kcmdline.c
+++ b/src/appliance-kcmdline.c
@@ -36,23 +36,8 @@
  * Check that the $TERM environment variable is reasonable before
  * we pass it through to the appliance.
  */
-static bool
-valid_term (const char *term)
-{
-  size_t len = strlen (term);
-
-  if (len == 0 || len > 16)
-    return false;
-
-  while (len > 0) {
-    char c = *term++;
-    len--;
-    if (!c_isalnum (c) && c != '-' && c != '_')
-      return false;
-  }
-
-  return true;
-}
+#define VALID_TERM(term) \
+  guestfs_int_string_is_valid ((term), 1, 16, true, true, "-_")
 
 #if defined(__powerpc64__)
 #define SERIAL_CONSOLE "console=hvc0 console=ttyS0"
@@ -196,7 +181,7 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char
*appliance_dev,
     guestfs_int_add_string (g, &argv, "guestfs_network=1");
 
   /* TERM environment variable. */
-  if (term && valid_term (term))
+  if (term && VALID_TERM (term))
     guestfs_int_add_sprintf (g, &argv, "TERM=%s", term);
   else
     guestfs_int_add_string (g, &argv, "TERM=linux");
diff --git a/src/drives.c b/src/drives.c
index 1e04f81..594f019 100644
--- a/src/drives.c
+++ b/src/drives.c
@@ -588,65 +588,22 @@ guestfs_int_free_drives (guestfs_h *g)
  * Check string parameter matches regular expression
  * C<^[-_[:alnum:]]+$> (in C locale).
  */
-static int
-valid_format_iface (const char *str)
-{
-  size_t len = strlen (str);
-
-  if (len == 0)
-    return 0;
-
-  while (len > 0) {
-    char c = *str++;
-    len--;
-    if (c != '-' && c != '_' && !c_isalnum (c))
-      return 0;
-  }
-  return 1;
-}
+#define VALID_FORMAT_IFACE(str) \
+  guestfs_int_string_is_valid ((str), 1, SIZE_MAX, true, true, "-_")
 
 /**
  * Check the disk label is reasonable.  It can't contain certain
  * characters, eg. C<'/'>, C<','>.  However be
stricter here and
  * ensure it's just alphabetic and E<le> 20 characters in length.
  */
-static int
-valid_disk_label (const char *str)
-{
-  size_t len = strlen (str);
-
-  if (len == 0 || len > 20)
-    return 0;
-
-  while (len > 0) {
-    char c = *str++;
-    len--;
-    if (!c_isalpha (c))
-      return 0;
-  }
-  return 1;
-}
+#define VALID_DISK_LABEL(str) \
+  guestfs_int_string_is_valid ((str), 1, 20, true, false, NULL)
 
 /**
  * Check the server hostname is reasonable.
  */
-static int
-valid_hostname (const char *str)
-{
-  size_t len = strlen (str);
-
-  if (len == 0 || len > 255)
-    return 0;
-
-  while (len > 0) {
-    char c = *str++;
-    len--;
-    if (!c_isalnum (c) &&
-        c != '-' && c != '.' && c !=
':' && c != '[' && c != ']')
-      return 0;
-  }
-  return 1;
-}
+#define VALID_HOSTNAME(str) \
+  guestfs_int_string_is_valid ((str), 1, 255, true, true, "-.:[]")
 
 /**
  * Check the port number is reasonable.
@@ -700,7 +657,7 @@ parse_one_server (guestfs_h *g, const char *server, struct
drive_server *ret)
       return -1;
     }
     free (port_str);
-    if (!valid_hostname (hostname)) {
+    if (!VALID_HOSTNAME (hostname)) {
       error (g, _("invalid hostname '%s'"), hostname);
       free (hostname);
       return -1;
@@ -711,7 +668,7 @@ parse_one_server (guestfs_h *g, const char *server, struct
drive_server *ret)
   }
 
   /* Doesn't match anything above, so assume it's a bare hostname. */
-  if (!valid_hostname (server)) {
+  if (!VALID_HOSTNAME (server)) {
     error (g, _("invalid hostname or server string '%s'"),
server);
     return -1;
   }
@@ -814,19 +771,19 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char
*filename,
     return -1;
   }
 
-  if (data.format && !valid_format_iface (data.format)) {
+  if (data.format && !VALID_FORMAT_IFACE (data.format)) {
     error (g, _("%s parameter is empty or contains disallowed
characters"),
            "format");
     free_drive_servers (data.servers, data.nr_servers);
     return -1;
   }
-  if (data.iface && !valid_format_iface (data.iface)) {
+  if (data.iface && !VALID_FORMAT_IFACE (data.iface)) {
     error (g, _("%s parameter is empty or contains disallowed
characters"),
            "iface");
     free_drive_servers (data.servers, data.nr_servers);
     return -1;
   }
-  if (data.disk_label && !valid_disk_label (data.disk_label)) {
+  if (data.disk_label && !VALID_DISK_LABEL (data.disk_label)) {
     error (g, _("label parameter is empty, too long, or contains
disallowed characters"));
     free_drive_servers (data.servers, data.nr_servers);
     return -1;
diff --git a/src/guestfs-internal-frontend.h b/src/guestfs-internal-frontend.h
index bc57506..ebc1f1b 100644
--- a/src/guestfs-internal-frontend.h
+++ b/src/guestfs-internal-frontend.h
@@ -32,6 +32,8 @@
 #ifndef GUESTFS_INTERNAL_FRONTEND_H_
 #define GUESTFS_INTERNAL_FRONTEND_H_
 
+#include <stdbool.h>
+
 #include "guestfs-internal-all.h"
 
 #define _(str) dgettext(PACKAGE, (str))
@@ -89,6 +91,7 @@ extern int guestfs_int_random_string (char *ret, size_t len);
 extern char *guestfs_int_drive_name (size_t index, char *ret);
 extern ssize_t guestfs_int_drive_index (const char *);
 extern int guestfs_int_is_true (const char *str);
+extern bool guestfs_int_string_is_valid (const char *str, size_t min_length,
size_t max_length, bool alpha, bool digit, const char *extra);
 //extern void guestfs_int_fadvise_normal (int fd);
 extern void guestfs_int_fadvise_sequential (int fd);
 extern void guestfs_int_fadvise_random (int fd);
diff --git a/src/unit-tests.c b/src/unit-tests.c
index 06d4d27..e22a61e 100644
--- a/src/unit-tests.c
+++ b/src/unit-tests.c
@@ -433,6 +433,45 @@ test_stringsbuf (void)
   guestfs_close (g);
 }
 
+/* Use the same macros as in src/drives.c */
+#define VALID_FORMAT_IFACE(str) \
+  guestfs_int_string_is_valid ((str), 1, SIZE_MAX, true, true, "-_")
+#define VALID_DISK_LABEL(str) \
+  guestfs_int_string_is_valid ((str), 1, 20, true, false, NULL)
+#define VALID_HOSTNAME(str) \
+  guestfs_int_string_is_valid ((str), 1, 255, true, true, "-.:[]")
+
+static void
+test_valid (void)
+{
+  assert (!VALID_FORMAT_IFACE (""));
+  assert (!VALID_DISK_LABEL (""));
+  assert (!VALID_HOSTNAME (""));
+
+  assert (!VALID_DISK_LABEL ("012345678901234567890"));
+
+  assert (VALID_FORMAT_IFACE ("abc"));
+  assert (VALID_FORMAT_IFACE ("ABC"));
+  assert (VALID_FORMAT_IFACE ("abc123"));
+  assert (VALID_FORMAT_IFACE ("abc123-"));
+  assert (VALID_FORMAT_IFACE ("abc123_"));
+  assert (!VALID_FORMAT_IFACE ("abc123."));
+
+  assert (VALID_DISK_LABEL ("abc"));
+  assert (VALID_DISK_LABEL ("ABC"));
+  assert (!VALID_DISK_LABEL ("abc123"));
+  assert (!VALID_DISK_LABEL ("abc123-"));
+
+  assert (VALID_HOSTNAME ("abc"));
+  assert (VALID_HOSTNAME ("ABC"));
+  assert (VALID_HOSTNAME ("abc123"));
+  assert (VALID_HOSTNAME ("abc-123"));
+  assert (VALID_HOSTNAME ("abc.123"));
+  assert (VALID_HOSTNAME ("abc:123"));
+  assert (VALID_HOSTNAME ("abc[123]"));
+  assert (!VALID_HOSTNAME ("abc/def"));
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -448,6 +487,7 @@ main (int argc, char *argv[])
   test_timeval_diff ();
   test_match ();
   test_stringsbuf ();
+  test_valid ();
 
   exit (EXIT_SUCCESS);
 }
diff --git a/src/utils.c b/src/utils.c
index 0b8962c..6a01ac8 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -27,6 +27,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>
 #include <string.h>
 #include <unistd.h>
 #include <fcntl.h>
@@ -37,9 +38,11 @@
 
 /* NB: MUST NOT require linking to gnulib, because that will break the
  * Python 'sdist' which includes a copy of this file.  It's OK to
- * include "ignore-value.h" here (since it is a header only with no
- * other code), but we also had to copy this file to the Python sdist.
+ * include "c-ctype.h" and "ignore-value.h" here (since it
is a header
+ * only with no other code), but we also had to copy these files to
+ * the Python sdist.
  */
+#include "c-ctype.h"
 #include "ignore-value.h"
 
 /* NB: MUST NOT include "guestfs-internal.h". */
@@ -360,6 +363,49 @@ guestfs_int_is_true (const char *str)
   return -1;
 }
 
+/**
+ * Check a string for validity, that it contains only certain
+ * characters, and minimum and maximum length.  This function is
+ * usually wrapped in a VALID_* macro, see F<src/drives.c> for an
+ * example.
+ *
+ * C<str> is the string to check.
+ *
+ * C<min_length> and C<max_length> are the minimum and maximum
+ * length checks.
+ *
+ * C<alpha> means 7-bit ASCII-only alphabetic characters are
+ * permitted.  C<digit> means 7-bit ASCII-only digits are permitted.
+ * C<extra> is a set of extra characters permitted.
+ *
+ * Returns boolean C<true> if the string is valid (passes all the
+ * tests), or C<false> if not.
+ */
+bool
+guestfs_int_string_is_valid (const char *str,
+                             size_t min_length, size_t max_length,
+                             bool alpha, bool digit, const char *extra)
+{
+  size_t i, len = strlen (str);
+
+  if ((min_length > 0 && len < min_length) ||
+      (max_length < SIZE_MAX && len > max_length))
+    return false;
+
+  for (i = 0; i < len; ++i) {
+    bool valid_char;
+
+    valid_char +      (alpha && c_isalpha (str[i])) ||
+      (digit && c_isdigit (str[i])) ||
+      (extra && strchr (extra, str[i]));
+
+    if (!valid_char) return false;
+  }
+
+  return true;
+}
+
 #if 0 /* not used yet */
 /**
  * Hint that we will read or write the file descriptor normally.
-- 
2.9.3

On Saturday, 24 December 2016 17:05:59 CET Richard W.M. Jones
wrote:
> ---
>  python/Makefile.am              |  5 ++++
>  src/appliance-kcmdline.c        | 21 ++-----------
>  src/drives.c                    | 65
+++++++----------------------------------
>  src/guestfs-internal-frontend.h |  3 ++
>  src/unit-tests.c                | 40 +++++++++++++++++++++++++
>  src/utils.c                     | 50 +++++++++++++++++++++++++++++--
>  6 files changed, 110 insertions(+), 74 deletions(-)
> 
Look reasonable, just a couple of notes.
> +bool
> +guestfs_int_string_is_valid (const char *str,
> +                             size_t min_length, size_t max_length,
> +                             bool alpha, bool digit, const char *extra)
min_length and max_length could be signed (ssize_t), with -1 (and even
0) indicating no actual limit. IMHO that could be slightly more readable
than SIZE_MAX.

Also, all the usages so far pass alpha=true, so could be worth just
assuming that for now, and dropping that parameter. After all, without
alpha chars there are two combinations:
a) a number
b) number + extra chars
(a) is already done by other functions converting string to numbers,
while (b) looks like an uncommon case, and not needed yet.

Thanks,
-- 
Pino Toscano