Subject: bug#23388: closed (Re: bug#23388: [PATCH] size option may want to reject "bB" or "biB" suffix)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 27 Apr 2016 19:13:02 +0000

Your bug report

#23388: [PATCH] size option may want to reject "bB" or "biB" suffix

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
23388: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23388
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#23388: [PATCH] size option may want to reject "bB" or "biB" suffix
From: Paul Eggert <[email-masked]>
To: Young Mo Kang <[email-masked]>, [email-masked]
Date: Wed, 27 Apr 2016 12:12:03 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 04/27/2016 09:29 AM, Young Mo Kang wrote:
> different programs within coreutils accept different
> size suffixes. For example, split's valid suffix is "bEGKkMmPTYZ0" while
> shred's is "cbBkKMGTPEZY0". I thought maybe it is better to unify valid
> suffix for all the programs within coreutils.

The intent, as I recall, was to prefer a standard set of suffixes listed 
in the "Block size" section of the Coreutils manual. Some programs 
accept additional suffixes for historical reasons but we'd rather not 
encourage the use of these obsolescent usages.

Thanks for reporting the glitch. I installed the attached patch into Gnulib.

Subject: [PATCH] size option may want to reject "bB" or "biB" suffix
From: Young Mo Kang <[email-masked]>
To: [email-masked]
Date: Thu, 28 Apr 2016 01:29:21 +0900
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello,

I found a bug in coreutils' size option, which currently accepts options
like "split -b 1bB" or "split -b 1biB". I believe these options should be
rejected. I looked through the code and found out that gnulib's __xstrtol
function in xstrtol.c is the culprit. I did a quick fix and the patch is
attached. The patch should fix this issue in general.

Additionally, while looking at the code, I may have found another bug, but
I am not so sure whether this is how it is intended. When I run "shred -s
1B", I think it should shred only a single byte, but it seems to shred 1024
bytes instead. Is this behavior intended?

Anyways, I have marked this down in the patch as a FIXME comment. Since the
patch applies to gnulib, I am not sure whether this patch should be
submitted to gnulib bug report instead. Please let me know if so.

Lastly, I noticed that different programs within coreutils accept different
size suffixes. For example, split's valid suffix is "bEGKkMmPTYZ0" while
shred's is "cbBkKMGTPEZY0". I thought maybe it is better to unify valid
suffix for all the programs within coreutils.

Best,
Young Mo
From 9290639bb9fd8bc999d84b8422a344c3eb990f2c Mon Sep 17 00:00:00 2001
From: Young Mo Kang <[email-masked]>
Date: Thu, 28 Apr 2016 00:41:12 +0900
Subject: [PATCH] Reject unaccepted suffixes, such as 'bB' or 'biB'

* xstrtol.c (__xstrtol): When optional second suffix is allowed,
check the first suffix for 'c', 'b', and 'w'. Produce an error
when these are followed by the second suffix.
---
 lib/xstrtol.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/xstrtol.c b/lib/xstrtol.c
index 5a3afb5..b117382 100644
--- a/lib/xstrtol.c
+++ b/lib/xstrtol.c
@@ -157,6 +157,20 @@ __xstrtol (const char *s, char **ptr, int strtol_base,
              a power of 1024.  If no suffix (e.g. "100M"), assume
              power-of-1024.  */
 
+         if (p[0][1] != '\0')
+         {
+           switch (**p)
+           {
+             case 'b':
+             case 'c':
+             case 'w': /* When the second suffix is allowed,
+                          'b', 'c', or 'w' shall not be the first suffix.
+                          For example, 'biB', 'bB', or 'bD' shall return error */
+               *val = tmp;
+               return err | LONGINT_INVALID_SUFFIX_CHAR;
+           }
+         }
+
           switch (p[0][1])
             {
             case 'i':
@@ -179,6 +193,8 @@ __xstrtol (const char *s, char **ptr, int strtol_base,
           break;
 
         case 'B':
+          /* FIXME: Is this correct? Doesn't 'B' represent a single byte,
+           *        which should be equivalent to 'c'? */
           overflow = bkm_scale (&tmp, 1024);
           break;
 
-- 
2.7.4