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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17455 in the body.
You can then email your comments to 17455 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> CS.UCLA.EDU>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] shred: fix overflow checking of command-line options
Date: Sat, 10 May 2014 11:42:38 -0700

* 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
@@ -1231,7 +1231,7 @@ main (int argc, char **argv)
           {
             uintmax_t tmp;
             if (xstrtoumax (optarg, NULL, 10, &tmp, NULL) != LONGINT_OK
-                || MIN (UINT32_MAX, SIZE_MAX / sizeof (int)) < tmp)
+                || MIN (ULONG_MAX, SIZE_MAX / sizeof (int)) <= tmp)
               {
                 error (EXIT_FAILURE, 0, _("%s: invalid number of passes"),
                        quotearg_colon (optarg));
@@ -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)
               {
                 error (EXIT_FAILURE, 0, _("%s: invalid file size"),
                        quotearg_colon (optarg));
-- 
1.9.0

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

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 17455 <at> debbugs.gnu.org
Subject: Re: bug#17455: [PATCH] shred: fix overflow checking of command-line
 options
Date: Sat, 10 May 2014 12:39:44 -0700

[Message part 1 (text/plain, inline)]

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 (application/octet-stream, attachment)]

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> CS.UCLA.EDU>
Cc: 17455 <at> debbugs.gnu.org
Subject: Re: bug#17455: [PATCH] shred: fix overflow checking of command-line
 options
Date: Sat, 10 May 2014 20:53:52 +0100

On 05/10/2014 07:42 PM, Paul Eggert 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
> @@ -1231,7 +1231,7 @@ main (int argc, char **argv)
>            {
>              uintmax_t tmp;
>              if (xstrtoumax (optarg, NULL, 10, &tmp, NULL) != LONGINT_OK
> -                || MIN (UINT32_MAX, SIZE_MAX / sizeof (int)) < tmp)
> +                || MIN (ULONG_MAX, SIZE_MAX / sizeof (int)) <= tmp)

Cool. I think the UNIT32 came from the first version
where ((word32)passes != passes) was used as a hacky
way to detect overflow.

>                {
>                  error (EXIT_FAILURE, 0, _("%s: invalid number of passes"),
>                         quotearg_colon (optarg));
> @@ -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)
>                {
>                  error (EXIT_FAILURE, 0, _("%s: invalid file size"),
>                         quotearg_colon (optarg));
> 

Ever since --size was added, it was stored and interpreted internally as off_t,
so the OFF_T_MAX guard is correct.
However the xstrtoumax() was used as a guard against negative numbers
which are now accepted :( I'll fixup with a revert to that bit.

thanks!
Pádraig.

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.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 17455 <at> debbugs.gnu.org
Subject: Re: bug#17455: [PATCH] shred: fix overflow checking of command-line
 options
Date: Sat, 10 May 2014 13:05:45 -0700

Thanks for catching that!

I used signed because I was worried about offbeat hosts where 
UINTMAX_MAX < OFF_T_MAX, which I think POSIX allows, but I suppose I was 
going overboard.

There are still a few oddball hosts out there (I just the other day 
discovered that Unisys still ships POSIXish mainframes where UINTMAX_MAX 
== OFF_T_MAX, because off_t has 40 bits and uintmax_t has 39!) but 
nothing *that* weird as far as I know.

Previous Next

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