Libguestfs - Feb 2018 - [RFC nbdkit PATCH] utils: Revamp nbdkit_parse_size

The existing implementation admitted that it was not very robust
in the face of garbage input; tighten things up by reimplementing
the function, and add testsuite coverage to ensure further tweaks
do not break things.

The testsuite additions were interesting - we didn't have any easy
way to link against a subset of the src/ files (all previous uses
of the util functions have been through dynamic .so libraries,
rather than standalone binaries).

Signed-off-by: Eric Blake <eblake@redhat.com>

---

RFC, because I don't know if I like my testsuite additions; automake
in particular was rather noisy about it:

tests/Makefile.am:99: warning: source file '$(top_srcdir)/src/utils.c'
is in a subdirectory,
tests/Makefile.am:99: but option 'subdir-objects' is disabled
automake-1.15: warning: possible forward-incompatibility.
automake-1.15: At least a source file is in a subdirectory, but the
'subdir-objects'
automake-1.15: automake option hasn't been enabled.  For now, the
corresponding output
automake-1.15: object file(s) will be placed in the top-level directory. 
However,
automake-1.15: this behaviour will change in future Automake versions: they will
automake-1.15: unconditionally cause object files to be placed in the same
subdirectory
automake-1.15: of the corresponding sources.
automake-1.15: You are advised to start using 'subdir-objects' option
throughout your
automake-1.15: project, to avoid future incompatibilities.

Maybe there's a smarter way to do this?

Or, I could just commit the src/utils.c change, and leave out the
testsuite additions.

---
 src/utils.c        |  95 ++++++++++++++++++++++++++---------------
 tests/Makefile.am  |  11 +++++
 tests/test-utils.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 .gitignore         |   1 +
 4 files changed, 195 insertions(+), 33 deletions(-)
 create mode 100644 tests/test-utils.c

diff --git a/src/utils.c b/src/utils.c
index 5663043..6e04bc8 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -80,49 +80,78 @@ nbdkit_absolute_path (const char *path)
   return ret;
 }

-/* XXX Multiple problems with this function.  Really we should use the
- * 'human*' functions from gnulib.
+/* Parse a string with possible scaling suffix, or return -1 after reporting
+ * the error.
  */
 int64_t
 nbdkit_parse_size (const char *str)
 {
   uint64_t size;
-  char t;
+  char *end;
+  uint64_t scale = 1;

-  if (sscanf (str, "%" SCNu64 "%c", &size, &t) ==
2) {
-    switch (t) {
-    case 'b': case 'B':
-      return (int64_t) size;
-    case 'k': case 'K':
-      return (int64_t) size * 1024;
-    case 'm': case 'M':
-      return (int64_t) size * 1024 * 1024;
-    case 'g': case 'G':
-      return (int64_t) size * 1024 * 1024 * 1024;
-    case 't': case 'T':
-      return (int64_t) size * 1024 * 1024 * 1024 * 1024;
-    case 'p': case 'P':
-      return (int64_t) size * 1024 * 1024 * 1024 * 1024 * 1024;
-    case 'e': case 'E':
-      return (int64_t) size * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
+  /* Scan unsigned, but range check later that result fits in signed. */
+  /* XXX Should we also parse things like '1.5M'? */
+  /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
+   * because some of them are valid hex digits */
+  errno = 0;
+  size = strtoumax (str, &end, 10);
+  if (errno || str == end) {
+    nbdkit_error ("could not parse size string (%s)", str);
+    return -1;
+  }
+  switch (*end) {
+  case '\0':
+    end--; /* Safe, since we already filtered out empty string */
+    break;
+
+    /* Powers of 1024 */
+  case 'e': case 'E':
+    scale *= 1024;
+    /* fallthru */
+  case 'p': case 'P':
+    scale *= 1024;
+    /* fallthru */
+  case 't': case 'T':
+    scale *= 1024;
+    /* fallthru */
+  case 'g': case 'G':
+    scale *= 1024;
+    /* fallthru */
+  case 'm': case 'M':
+    scale *= 1024;
+    /* fallthru */
+  case 'k': case 'K':
+    scale *= 1024;
+    /* fallthru */
+  case 'b': case 'B':
+    break;

-    case 's': case 'S':         /* "sectors", ie.
units of 512 bytes,
-                                 * even if that's not the real sector size
-                                 */
-      return (int64_t) size * 512;
+  case 's': case 'S':         /* "sectors", ie. units
of 512 bytes,
+                               * even if that's not the real sector size
+                               */
+    scale = 512;
+    break;

-    default:
-      nbdkit_error ("could not parse size: unknown specifier
'%c'", t);
-      return -1;
-    }
+  default:
+    nbdkit_error ("could not parse size: unknown suffix
'%s'", end);
+    return -1;
   }

-  /* bytes */
-  if (sscanf (str, "%" SCNu64, &size) == 1)
-    return (int64_t) size;
+  /* XXX Maybe we should support 'MiB' as synonym for 'M'; and
'MB'
+   * for powers of 1000, for similarity to GNU tools. But for now,
+   * anything beyond 'M' is dropped.  */
+  if (end[1]) {
+    nbdkit_error ("could not parse size: unknown suffix
'%s'", end);
+    return -1;
+  }
+
+  if (INT64_MAX / scale < size) {
+    nbdkit_error ("overflow computing size (%s)", str);
+    return -1;
+  }

-  nbdkit_error ("could not parse size string (%s)", str);
-  return -1;
+  return size * scale;
 }

 /* Read a password from configuration value. */
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1e32cb6..baae6a1 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -90,8 +90,19 @@ TESTS_ENVIRONMENT = PATH=$(abs_top_builddir):$(PATH)
 TESTS = \
 	test-help.sh \
 	test-version.sh \
+	test-utils \
 	test-dump-config.sh

+check_PROGRAMS += \
+	test-utils
+
+test_utils_SOURCES = test-utils.c \
+	$(top_srcdir)/src/utils.c \
+	$(top_srcdir)/src/cleanup.c \
+	$(top_srcdir)/include/nbdkit-plugin.h
+test_utils_CPPFLAGS = -I$(top_srcdir)/include
+test_utils_CFLAGS = $(WARNINGS_CFLAGS)
+
 if HAVE_PLUGINS

 TESTS += \
diff --git a/tests/test-utils.c b/tests/test-utils.c
new file mode 100644
index 0000000..af08416
--- /dev/null
+++ b/tests/test-utils.c
@@ -0,0 +1,121 @@
+/* nbdkit
+ * Copyright (C) 2018 Red Hat Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <inttypes.h>
+#include <stdbool.h>
+
+#include <nbdkit-plugin.h>
+
+static bool error_flagged;
+
+/* Stub for linking against minimal source files */
+void
+nbdkit_error (const char *fs, ...)
+{
+  error_flagged = true;
+}
+
+static bool
+test_nbdkit_parse_size (void)
+{
+  bool pass = true;
+  struct pair {
+    const char *str;
+    int64_t res;
+  } pairs[] = {
+    /* Bogus strings */
+    { "", -1 },
+    { "0x0", -1 },
+    { "garbage", -1 },
+    { "0garbage", -1 },
+    { "M", -1 },
+    { "-1", -1 },
+    { "-2", -1 },
+    { "9223372036854775808", -1 },
+    { "-9223372036854775808", -1 },
+    { "8E", -1 },
+    { "8192P", -1 },
+
+    /* Valid strings */
+    { "0", 0 },
+    { " 08", 8 },
+    { "9223372036854775807", INT64_MAX },
+    { "1s", 512 },
+    { "2S", 1024 },
+    { "1b", 1 },
+    { "1B", 1 },
+    { "1k", 1024 },
+    { "1K", 1024 },
+    { "1m", 1024 * 1024 },
+    { "1M", 1024 * 1024 },
+    { "1g", 1024 * 1024 * 1024 },
+    { "1G", 1024 * 1024 * 1024 },
+    { "1t", 1024LL * 1024 * 1024 * 1024 },
+    { "1T", 1024LL * 1024 * 1024 * 1024 },
+    { "1p", 1024LL * 1024 * 1024 * 1024 * 1024 },
+    { "1P", 1024LL * 1024 * 1024 * 1024 * 1024 },
+    { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+    { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+  };
+
+  for (size_t i = 0; i < sizeof pairs / sizeof pairs[0]; i++) {
+    int64_t r;
+
+    error_flagged = false;
+    r = nbdkit_parse_size (pairs[i].str);
+    if (r != pairs[i].res) {
+      fprintf (stderr,
+               "Wrong parse for %s, got %" PRId64 ", expected
%" PRId64 "\n",
+               pairs[i].str, r, pairs[i].res);
+      pass = false;
+    }
+    if ((r == -1) != error_flagged) {
+      fprintf (stderr, "Wrong error message handling for %s\n",
pairs[i].str);
+      pass = false;
+    }
+  }
+
+  return pass;
+}
+
+int
+main (int argc, char *argv[])
+{
+  bool pass = true;
+  pass &= test_nbdkit_parse_size ();
+  /* XXX tests nbdkit_absolute_path, nbdkit_read_password */
+  return pass ? 0 : 1;
+}
diff --git a/.gitignore b/.gitignore
index 09ed262..28215bc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,5 +72,6 @@ Makefile.in
 /tests/test-socket-activation
 /tests/test-split
 /tests/test-streaming
+/tests/test-utils
 /tests/test-xz
 /test-driver
-- 
2.14.3