[Libguestfs] [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(a)redhat.com&gt; --- 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