Libguestfs - Aug 2014 - [PATCH] rescue: fix sscanf placeholders for --smp and --memsize

Use %d to parse them as int (since the variables for them as int)
instead of %u, even if they both need to be at least > 0.

--smp was already checked to be >= 1 while --memsize not, so check that
the specified memory size is not < 128 (which is semi-arbitrary, but
enough as a minimum threshold).
---
 rescue/rescue.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/rescue/rescue.c b/rescue/rescue.c
index dc56d4b..1c556e5 100644
--- a/rescue/rescue.c
+++ b/rescue/rescue.c
@@ -157,7 +157,7 @@ main (int argc, char *argv[])
         else
           format = optarg;
       } else if (STREQ (long_options[option_index].name, "smp")) {
-        if (sscanf (optarg, "%u", &smp) != 1) {
+        if (sscanf (optarg, "%d", &smp) != 1) {
           fprintf (stderr, _("%s: could not parse --smp parameter
'%s'\n"),
                    program_name, optarg);
           exit (EXIT_FAILURE);
@@ -208,11 +208,19 @@ main (int argc, char *argv[])
       break;
 
     case 'm':
-      if (sscanf (optarg, "%u", &memsize) != 1) {
+      if (sscanf (optarg, "%d", &memsize) != 1) {
         fprintf (stderr, _("%s: could not parse memory size
'%s'\n"),
                  program_name, optarg);
         exit (EXIT_FAILURE);
       }
+      /* Check with a semi-arbitrary value, but avoids too small values
+       * (including <= 0)
+       */
+      if (memsize < 128) {
+        fprintf (stderr, _("%s: memory size %d is too small\n"),
+                 program_name, memsize);
+        exit (EXIT_FAILURE);
+      }
       break;
 
     case 'r':
-- 
1.9.3

On Thu, Aug 07, 2014 at 07:48:07PM +0200, Pino Toscano
wrote:
> Use %d to parse them as int (since the variables for them as int)
> instead of %u, even if they both need to be at least > 0.
> 
> --smp was already checked to be >= 1 while --memsize not, so check that
> the specified memory size is not < 128 (which is semi-arbitrary, but
> enough as a minimum threshold).
[...]
> +      /* Check with a semi-arbitrary value, but avoids too small values
> +       * (including <= 0)
> +       */
> +      if (memsize < 128) {
> +        fprintf (stderr, _("%s: memory size %d is too small\n"),
> +                 program_name, memsize);
> +        exit (EXIT_FAILURE);
> +      }
Interestingly the library checks that memsize >= MIN_MEMSIZE, but only
when you set it through the environment variable.  I suspect the
library should check it in guestfs__set_memsize too, instead of having
to have checks scattered throughout API users.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Move the minimum memory check from the environment parsing to
set_memsize, so the limit is actually enforced also when using the API.

Adapt the rhbz557655.sh test to the invalid memsize values being
rejected now, and add a new test for checking invalid parameters
explicitly.
---
 fish/Makefile.am                             |  1 +
 fish/test-invalid-params.sh                  | 71 ++++++++++++++++++++++++++++
 src/handle.c                                 | 13 +++--
 tests/regressions/rhbz557655-expected.stdout |  7 ++-
 tests/regressions/rhbz557655.sh              |  8 ++--
 5 files changed, 88 insertions(+), 12 deletions(-)
 create mode 100755 fish/test-invalid-params.sh

diff --git a/fish/Makefile.am b/fish/Makefile.am
index ad984ec..9ce1daf 100644
--- a/fish/Makefile.am
+++ b/fish/Makefile.am
@@ -272,6 +272,7 @@ TESTS = \
 	test-d.sh \
 	test-escapes.sh \
 	test-events.sh \
+	test-invalid-params.sh \
 	test-tilde.sh
 
 if ENABLE_APPLIANCE
diff --git a/fish/test-invalid-params.sh b/fish/test-invalid-params.sh
new file mode 100755
index 0000000..60da094
--- /dev/null
+++ b/fish/test-invalid-params.sh
@@ -0,0 +1,71 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2014 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test passing invalid parameters for memory size, smp, etc when setting up
+# the appliance.
+
+set -e
+
+# Memory size
+output=$(
+$VG ./guestfish <<EOF
+set-memsize 400
+-set-memsize 0
+-set-memsize 100
+-set-memsize -500
+-set-memsize 0x10
+-set-memsize 010
+-set-memsize -1073741824
+get-memsize
+EOF
+)
+if [ "$output" != "400" ]; then
+    echo "$0: error: output of guestfish after memsize commands did not
match expected output"
+    echo "$output"
+    exit 1
+fi
+
+# smp
+output=$(
+$VG ./guestfish <<EOF
+set-smp 2
+-set-smp 0
+-set-smp 300
+-set-smp -2
+get-smp
+EOF
+)
+if [ "$output" != "2" ]; then
+    echo "$0: error: output of guestfish after smp commands did not match
expected output"
+    echo "$output"
+    exit 1
+fi
+
+# Backend
+output=$(
+$VG ./guestfish <<EOF
+set-backend direct
+-set-backend backend-which-does-not-exist
+get-backend
+EOF
+)
+if [ "$output" != "direct" ]; then
+    echo "$0: error: output of guestfish after backend commands did not
match expected output"
+    echo "$output"
+    exit 1
+fi
diff --git a/src/handle.c b/src/handle.c
index 719bbcd..0200528 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -226,11 +226,14 @@ parse_environment (guestfs_h *g,
 
   str = do_getenv (data, "LIBGUESTFS_MEMSIZE");
   if (str) {
-    if (sscanf (str, "%d", &memsize) != 1 || memsize <
MIN_MEMSIZE) {
-      error (g, _("non-numeric or too small value for
LIBGUESTFS_MEMSIZE"));
+    if (sscanf (str, "%d", &memsize) != 1) {
+      error (g, _("non-numeric value for LIBGUESTFS_MEMSIZE"));
+      return -1;
+    }
+    if (guestfs_set_memsize (g, memsize) == -1) {
+      /* set_memsize produces an error message already. */
       return -1;
     }
-    guestfs_set_memsize (g, memsize);
   }
 
   str = do_getenv (data, "LIBGUESTFS_BACKEND");
@@ -552,6 +555,10 @@ guestfs__get_append (guestfs_h *g)
 int
 guestfs__set_memsize (guestfs_h *g, int memsize)
 {
+  if (memsize < MIN_MEMSIZE) {
+    error (g, _("too small value for memsize (must be at least %d)"),
MIN_MEMSIZE);
+    return -1;
+  }
   g->memsize = memsize;
   return 0;
 }
diff --git a/tests/regressions/rhbz557655-expected.stdout
b/tests/regressions/rhbz557655-expected.stdout
index 80bc8bc..abc08e0 100644
--- a/tests/regressions/rhbz557655-expected.stdout
+++ b/tests/regressions/rhbz557655-expected.stdout
@@ -1,7 +1,6 @@
-0
-16
-8
--1073741824
+500
+512
+4096
 1073741823
 1234
 1234
diff --git a/tests/regressions/rhbz557655.sh b/tests/regressions/rhbz557655.sh
index 750263b..87eb279 100755
--- a/tests/regressions/rhbz557655.sh
+++ b/tests/regressions/rhbz557655.sh
@@ -27,13 +27,11 @@ export LANG=C
 ../../fish/guestfish >> rhbz557655.out 2>> rhbz557655.err
<<EOF
 # set-memsize is just a convenient non-daemon function that
 # takes a single integer argument.
-set-memsize 0
+set-memsize 500
 get-memsize
-set-memsize 0x10
+set-memsize 0x200
 get-memsize
-set-memsize 010
-get-memsize
-set-memsize -1073741824
+set-memsize 010000
 get-memsize
 set-memsize 1073741823
 get-memsize
-- 
1.9.3

Use %d to parse them as int (since the variables for them as int)
instead of %u, even if they both need to be at least > 0; the library
will check for the validity of them anyway.
---
 rescue/rescue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rescue/rescue.c b/rescue/rescue.c
index dc56d4b..c73db55 100644
--- a/rescue/rescue.c
+++ b/rescue/rescue.c
@@ -157,7 +157,7 @@ main (int argc, char *argv[])
         else
           format = optarg;
       } else if (STREQ (long_options[option_index].name, "smp")) {
-        if (sscanf (optarg, "%u", &smp) != 1) {
+        if (sscanf (optarg, "%d", &smp) != 1) {
           fprintf (stderr, _("%s: could not parse --smp parameter
'%s'\n"),
                    program_name, optarg);
           exit (EXIT_FAILURE);
@@ -208,7 +208,7 @@ main (int argc, char *argv[])
       break;
 
     case 'm':
-      if (sscanf (optarg, "%u", &memsize) != 1) {
+      if (sscanf (optarg, "%d", &memsize) != 1) {
         fprintf (stderr, _("%s: could not parse memory size
'%s'\n"),
                  program_name, optarg);
         exit (EXIT_FAILURE);
-- 
1.9.3