[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
On Fri, Aug 08, 2014 at 11:29:24AM +0200, Pino Toscano wrote:
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
ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
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
On Fri, Aug 08, 2014 at 11:29:25AM +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; 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);
ACK. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
participants (2)
-
Pino Toscano -
Richard W.M. Jones