GNU bug report logs - #17455
[PATCH] shred: fix overflow checking of command-line options

Previous Next

Full log

Message #16 received at 17455-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, 17455-done <at> debbugs.gnu.org
Subject: Re: bug#17455: [PATCH] shred: fix overflow checking of command-line
 options
Date: Sat, 10 May 2014 20:57:08 +0100

On 05/10/2014 08:39 PM, Jim Meyering wrote:
> On Sat, May 10, 2014 at 11:42 AM, Paul Eggert <eggert <at> cs.ucla.edu> wrote:
>> > * src/shred.c (main): Limit -n (number of passes) value to
>> > ULONG_MAX, not to UINT32_MAX, since the vars are unsigned long.
>> > Limit the -s (file size) value to OFF_T_MAX.
>> > ---
>> >  src/shred.c | 9 +++++----
>> >  1 file changed, 5 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/src/shred.c b/src/shred.c
>> > index 607c6be..f4347e0 100644
>> > --- a/src/shred.c
>> > +++ b/src/shred.c
> ...
>> > @@ -1256,9 +1256,10 @@ main (int argc, char **argv)
>> >
>> >          case 's':
>> >            {
>> > -            uintmax_t tmp;
>> > -            if (xstrtoumax (optarg, NULL, 0, &tmp, "cbBkKMGTPEZY0")
>> > -                != LONGINT_OK)
>> > +            intmax_t tmp;
>> > +            if ((xstrtoimax (optarg, NULL, 0, &tmp, "cbBkKMGTPEZY0")
>> > +                 != LONGINT_OK)
>> > +                || OFF_T_MAX < tmp)
> Hi Paul,
> The above makes it so shred now accepts a negative size.
> Before, that would be diagnosed as invalid:
> 
>    $ shred -s-1 k
>    shred: -1: invalid file size
> 
> With a size of -2, shred will write 64KB blocks forever -- or until it
> runs out of space.
> 
> Here's a patch to fix that and to add a test covering that case:
> 
> 
> 0001-shred-don-t-infloop-upon-negative-size.patch
> 
> 
> From d7cfcbef7eb2cd12ac83e5c1c123de2015adbef9 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> fb.com>
> Date: Sat, 10 May 2014 12:36:16 -0700
> Subject: [PATCH] shred: don't infloop upon negative size
> 
> * src/shred.c (main): With the preceding change, shred -s-2 FILE
> would write 64KB blocks forever -- or until disk full. This change
> makes shred reject a negative size.
> * tests/misc/shred-negative.sh: New file.
> * tests/local.mk (all_tests): Add it.
> ---
>  src/shred.c                  |  4 ++--
>  tests/local.mk               |  1 +
>  tests/misc/shred-negative.sh | 28 ++++++++++++++++++++++++++++
>  3 files changed, 31 insertions(+), 2 deletions(-)
>  create mode 100755 tests/misc/shred-negative.sh
> 
> diff --git a/src/shred.c b/src/shred.c
> index f4347e0..bd88e38 100644
> --- a/src/shred.c
> +++ b/src/shred.c
> @@ -1256,8 +1256,8 @@ main (int argc, char **argv)
> 
>          case 's':
>            {
> -            intmax_t tmp;
> -            if ((xstrtoimax (optarg, NULL, 0, &tmp, "cbBkKMGTPEZY0")
> +            uintmax_t tmp;
> +            if ((xstrtoumax (optarg, NULL, 0, &tmp, "cbBkKMGTPEZY0")
>                   != LONGINT_OK)
>                  || OFF_T_MAX < tmp)
>                {
> diff --git a/tests/local.mk b/tests/local.mk
> index 5286bfb..cd7da5b 100644
> --- a/tests/local.mk
> +++ b/tests/local.mk
> @@ -313,6 +313,7 @@ all_tests =					\
>    tests/misc/sha384sum.pl			\
>    tests/misc/sha512sum.pl			\
>    tests/misc/shred-exact.sh			\
> +  tests/misc/shred-negative.sh			\
>    tests/misc/shred-passes.sh			\
>    tests/misc/shred-remove.sh			\
>    tests/misc/shuf.sh				\
> diff --git a/tests/misc/shred-negative.sh b/tests/misc/shred-negative.sh
> new file mode 100755
> index 0000000..86cbf3e
> --- /dev/null
> +++ b/tests/misc/shred-negative.sh
> @@ -0,0 +1,28 @@
> +#!/bin/sh
> +# Exercise shred -s-3 FILE
> +
> +# Copyright (C) 2014 Free Software Foundation, 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 3 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, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
> +print_ver_ shred
> +
> +echo 'shred: -2: invalid file size' > exp || framework_failure_
> +echo 1234 > f || framework_failure_
> +
> +shred -s-2 f 2>err && fail=1
> +compare exp err || fail=1
> +
> +Exit $fail
> -- 2.0.0.rc0.38.g1697bf3

Ah great you beat me to it :)
Code looks good, as does the test.

Please push.

I've marked this bug as done.

thanks!
Pádraig.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.