Subject: bug#66698: I think hex decoding with basenc -d --base16 should be case-insensitive

From: Paul Eggert <[email-masked]>
Date: Wed, 25 Oct 2023 15:11:43 -0700

On 10/25/23 06:27, Pádraig Brady wrote:
> But thinking about it more we probably should allow lower case for base32.

Yes, I'm thinking the same.

While thinking (:-) I couldn't resist improving performance a bit by 
installing the attached. This also fixes an unlikely bug when isxdigit 
behavior is oddball.

From f4a59d453ebfa6dcaf2dd1a89a64c1fa05af7a85 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 25 Oct 2023 08:45:15 -0700
Subject: [PATCH 1/3] build: update gnulib submodule to latest

---
 gnulib | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnulib b/gnulib
index b8083ec2e..e0ae1a7f3 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit b8083ec2eeb22c20d299d75aae06e465cf9e4494
+Subproject commit e0ae1a7f324d6b9462735273bc5a2848c712f883
-- 
2.41.0

From dcc1514d9ac12a98bf3067c206ce8e756c604719 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 25 Oct 2023 14:43:32 -0700
Subject: [PATCH 2/3] basenc: tweak checks to use unsigned char

This tends to generate better code, at least on x86-64,
because callers are just as fast and callees can avoid a conversion.
* src/basenc.c: The following renamings also change the arg type
from char to unsigned char.  All uses changed.
(isubase): Rename from isbase.
(isubase64url): Rename from isbase64url.
(isubase32hex): Rename from isbase32hex.
(isubase16): Rename from isbase16.
(isuz85): Rename from isz85.
(isubase2): Rename from isbase2.

2023-10-24  Paul Eggert  <[email-masked]>

* src/basenc.c (struct base16_decode_context):
Simplify by storing -1 for missing nibbles.  All uses changed.
---
 src/basenc.c | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/src/basenc.c b/src/basenc.c
index b47bd013f..850dd4cd9 100644
--- a/src/basenc.c
+++ b/src/basenc.c
@@ -216,7 +216,7 @@ static_assert (DEC_BLOCKSIZE % 40 == 0); /* Complete encoded blocks are used. */
 # define base_decode_context base32_decode_context
 # define base_decode_ctx_init base32_decode_ctx_init
 # define base_decode_ctx base32_decode_ctx
-# define isbase isbase32
+# define isubase isubase32
 #elif BASE_TYPE == 64
 # define BASE_LENGTH BASE64_LENGTH
 # define REQUIRED_PADDING base64_required_padding
@@ -232,7 +232,7 @@ static_assert (DEC_BLOCKSIZE % 12 == 0); /* Complete encoded blocks are used. */
 # define base_decode_context base64_decode_context
 # define base_decode_ctx_init base64_decode_ctx_init
 # define base_decode_ctx base64_decode_ctx
-# define isbase isbase64
+# define isubase isubase64
 #elif BASE_TYPE == 42

@@ -247,7 +247,7 @@ static_assert (DEC_BLOCKSIZE % 12 == 0); /* complete encoded blocks for base64*/
 
 static int (*base_length) (int i);
 static int (*required_padding) (int i);
-static bool (*isbase) (char ch);
+static bool (*isubase) (unsigned char ch);
 static void (*base_encode) (char const *restrict in, idx_t inlen,
                             char *restrict out, idx_t outlen);
 
@@ -350,10 +350,10 @@ base64url_encode (char const *restrict in, idx_t inlen,
 }
 
 static bool
-isbase64url (char ch)
+isubase64url (unsigned char ch)
 {
   return (ch == '-' || ch == '_'
-          || (ch != '+' && ch != '/' && isbase64 (ch)));
+          || (ch != '+' && ch != '/' && isubase64 (ch)));
 }
 
 static void
@@ -463,7 +463,7 @@ static const char base32_hex_to_norm[32 + 9] = {

 inline static bool
-isbase32hex (char ch)
+isubase32hex (unsigned char ch)
 {
   return ('0' <= ch && ch <= '9') || ('A' <= ch && ch <= 'V');
 }
@@ -502,8 +502,8 @@ base32hex_decode_ctx_wrapper (struct base_decode_context *ctx,
   char *p = ctx->inbuf;
   while (i--)
     {
-      if (isbase32hex (*in))
-        *p = base32_hex_to_norm[ (int)*in - 0x30];
+      if (isubase32hex (*in))
+        *p = base32_hex_to_norm[*in - 0x30];
       else
         *p = *in;
       ++p;
@@ -518,9 +518,9 @@ base32hex_decode_ctx_wrapper (struct base_decode_context *ctx,
 }
 
 static bool
-isbase16 (char ch)
+isubase16 (unsigned char ch)
 {
-  return isxdigit (to_uchar (ch));
+  return isxdigit (ch);
 }
 
 static int
@@ -614,7 +614,7 @@ z85_length (int len)
 }
 
 static bool
-isz85 (char ch)
+isuz85 (unsigned char ch)
 {
   return c_isalnum (ch) || strchr (".-:+=^!/*?&<>()[]{}@%$#", ch) != nullptr;
 }
@@ -796,7 +796,7 @@ z85_decode_ctx (struct base_decode_context *ctx,

 inline static bool
-isbase2 (char ch)
+isubase2 (unsigned char ch)
 {
   return ch == '0' || ch == '1';
 }
@@ -875,7 +875,7 @@ base2lsbf_decode_ctx (struct base_decode_context *ctx,
           continue;
         }
 
-      if (!isbase2 (*in))
+      if (!isubase2 (*in))
         return false;
 
       bool bit = (*in == '1');
@@ -919,7 +919,7 @@ base2msbf_decode_ctx (struct base_decode_context *ctx,
           continue;
         }
 
-      if (!isbase2 (*in))
+      if (!isubase2 (*in))
         return false;
 
       bool bit = (*in == '1');
@@ -1065,7 +1065,7 @@ do_decode (FILE *in, char const *infile, FILE *out, bool ignore_garbage)
             {
               for (idx_t i = 0; n > 0 && i < n;)
                 {
-                  if (isbase (inbuf[sum + i]) || inbuf[sum + i] == '=')
+                  if (isubase (inbuf[sum + i]) || inbuf[sum + i] == '=')
                     i++;
                   else
                     memmove (inbuf + sum + i, inbuf + sum + i + 1, --n - i);
@@ -1193,7 +1193,7 @@ main (int argc, char **argv)
     case BASE64_OPTION:
       base_length = base64_length_wrapper;
       required_padding = base64_required_padding;
-      isbase = isbase64;
+      isubase = isubase64;
       base_encode = base64_encode;
       base_decode_ctx_init = base64_decode_ctx_init_wrapper;
       base_decode_ctx = base64_decode_ctx_wrapper;
@@ -1202,7 +1202,7 @@ main (int argc, char **argv)
     case BASE64URL_OPTION:
       base_length = base64_length_wrapper;
       required_padding = base64_required_padding;
-      isbase = isbase64url;
+      isubase = isubase64url;
       base_encode = base64url_encode;
       base_decode_ctx_init = base64url_decode_ctx_init_wrapper;
       base_decode_ctx = base64url_decode_ctx_wrapper;
@@ -1211,7 +1211,7 @@ main (int argc, char **argv)
     case BASE32_OPTION:
       base_length = base32_length_wrapper;
       required_padding = base32_required_padding;
-      isbase = isbase32;
+      isubase = isubase32;
       base_encode = base32_encode;
       base_decode_ctx_init = base32_decode_ctx_init_wrapper;
       base_decode_ctx = base32_decode_ctx_wrapper;
@@ -1220,7 +1220,7 @@ main (int argc, char **argv)
     case BASE32HEX_OPTION:
       base_length = base32_length_wrapper;
       required_padding = base32_required_padding;
-      isbase = isbase32hex;
+      isubase = isubase32hex;
       base_encode = base32hex_encode;
       base_decode_ctx_init = base32hex_decode_ctx_init_wrapper;
       base_decode_ctx = base32hex_decode_ctx_wrapper;
@@ -1229,7 +1229,7 @@ main (int argc, char **argv)
     case BASE16_OPTION:
       base_length = base16_length;
       required_padding = no_required_padding;
-      isbase = isbase16;
+      isubase = isubase16;
       base_encode = base16_encode;
       base_decode_ctx_init = base16_decode_ctx_init;
       base_decode_ctx = base16_decode_ctx;
@@ -1238,7 +1238,7 @@ main (int argc, char **argv)
     case BASE2MSBF_OPTION:
       base_length = base2_length;
       required_padding = no_required_padding;
-      isbase = isbase2;
+      isubase = isubase2;
       base_encode = base2msbf_encode;
       base_decode_ctx_init = base2_decode_ctx_init;
       base_decode_ctx = base2msbf_decode_ctx;
@@ -1247,7 +1247,7 @@ main (int argc, char **argv)
     case BASE2LSBF_OPTION:
       base_length = base2_length;
       required_padding = no_required_padding;
-      isbase = isbase2;
+      isubase = isubase2;
       base_encode = base2lsbf_encode;
       base_decode_ctx_init = base2_decode_ctx_init;
       base_decode_ctx = base2lsbf_decode_ctx;
@@ -1256,7 +1256,7 @@ main (int argc, char **argv)
     case Z85_OPTION:
       base_length = z85_length;
       required_padding = no_required_padding;
-      isbase = isz85;
+      isubase = isuz85;
       base_encode = z85_encode;
       base_decode_ctx_init = z85_decode_ctx_init;
       base_decode_ctx = z85_decode_ctx;
-- 
2.41.0

From 60bd7bad9d2473d2bd0462a1cbe05144c35ad557 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 25 Oct 2023 15:09:04 -0700
Subject: [PATCH 3/3] basenc: fix unlikely locale issue; tune
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This sped up ‘basenc -d --base16’ by 60% on my old platform,
AMD Phenom II X4 910e, Fedora 38.
* src/basenc.c (struct base16_decode_context): Simplify by
omitting have_nibble.  ‘nibble’ is now negative if it’s missing.
All uses changed.
(B16): New macro, inspired by ../lib/base64.c.
(base16_to_int): New static var, likewise.
(isubase16): Reimplement using base16_to_int, since isxdigit is
not guaranteed to succeed on the chars we want when the locale is
oddball.
(base16_decode_ctx): Tune by using base16_to_int and by
---
 src/basenc.c | 144 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 118 insertions(+), 26 deletions(-)

diff --git a/src/basenc.c b/src/basenc.c
index 850dd4cd9..858e0cd5e 100644
--- a/src/basenc.c
+++ b/src/basenc.c
@@ -253,8 +253,8 @@ static void (*base_encode) (char const *restrict in, idx_t inlen,
 
 struct base16_decode_context
 {
-  char nibble;
-  bool have_nibble;
+  /* Either a 4-bit nibble, or negative if we have no nibble.  */
+  signed char nibble;
 };
 
 struct z85_decode_context
@@ -516,11 +516,105 @@ base32hex_decode_ctx_wrapper (struct base_decode_context *ctx,
 
   return b;
 }
+/* With this approach this file works independent of the charset used
+   (think EBCDIC).  However, it does assume that the characters in the
+   Base32 alphabet (A-Z2-7) are encoded in 0..255.  POSIX
+   1003.1-2001 require that char and unsigned char are 8-bit
+   quantities, though, taking care of that problem.  But this may be a
+   potential problem on non-POSIX C99 platforms.
+
+   IBM C V6 for AIX mishandles "#define B32(x) ...'x'...", so use "_"
+   as the formal parameter rather than "x".  */
+#define B16(_)                                  \
+  ((_) == '0' ? 0                               \
+   : (_) == '1' ? 1                             \
+   : (_) == '2' ? 2                             \
+   : (_) == '3' ? 3                             \
+   : (_) == '4' ? 4                             \
+   : (_) == '5' ? 5                             \
+   : (_) == '6' ? 6                             \
+   : (_) == '7' ? 7                             \
+   : (_) == '8' ? 8                             \
+   : (_) == '9' ? 9                             \
+   : (_) == 'A' || (_) == 'a' ? 10              \
+   : (_) == 'B' || (_) == 'b' ? 11              \
+   : (_) == 'C' || (_) == 'c' ? 12              \
+   : (_) == 'D' || (_) == 'd' ? 13              \
+   : (_) == 'E' || (_) == 'e' ? 14              \
+   : (_) == 'F' || (_) == 'f' ? 15              \
+   : -1)
+
+static signed char const base16_to_int[256] = {
+  B16 (0), B16 (1), B16 (2), B16 (3),
+  B16 (4), B16 (5), B16 (6), B16 (7),
+  B16 (8), B16 (9), B16 (10), B16 (11),
+  B16 (12), B16 (13), B16 (14), B16 (15),
+  B16 (16), B16 (17), B16 (18), B16 (19),
+  B16 (20), B16 (21), B16 (22), B16 (23),
+  B16 (24), B16 (25), B16 (26), B16 (27),
+  B16 (28), B16 (29), B16 (30), B16 (31),
+  B16 (32), B16 (33), B16 (34), B16 (35),
+  B16 (36), B16 (37), B16 (38), B16 (39),
+  B16 (40), B16 (41), B16 (42), B16 (43),
+  B16 (44), B16 (45), B16 (46), B16 (47),
+  B16 (48), B16 (49), B16 (50), B16 (51),
+  B16 (52), B16 (53), B16 (54), B16 (55),
+  B16 (56), B16 (57), B16 (58), B16 (59),
+  B16 (60), B16 (61), B16 (62), B16 (63),
+  B16 (32), B16 (65), B16 (66), B16 (67),
+  B16 (68), B16 (69), B16 (70), B16 (71),
+  B16 (72), B16 (73), B16 (74), B16 (75),
+  B16 (76), B16 (77), B16 (78), B16 (79),
+  B16 (80), B16 (81), B16 (82), B16 (83),
+  B16 (84), B16 (85), B16 (86), B16 (87),
+  B16 (88), B16 (89), B16 (90), B16 (91),
+  B16 (92), B16 (93), B16 (94), B16 (95),
+  B16 (96), B16 (97), B16 (98), B16 (99),
+  B16 (100), B16 (101), B16 (102), B16 (103),
+  B16 (104), B16 (105), B16 (106), B16 (107),
+  B16 (108), B16 (109), B16 (110), B16 (111),
+  B16 (112), B16 (113), B16 (114), B16 (115),
+  B16 (116), B16 (117), B16 (118), B16 (119),
+  B16 (120), B16 (121), B16 (122), B16 (123),
+  B16 (124), B16 (125), B16 (126), B16 (127),
+  B16 (128), B16 (129), B16 (130), B16 (131),
+  B16 (132), B16 (133), B16 (134), B16 (135),
+  B16 (136), B16 (137), B16 (138), B16 (139),
+  B16 (140), B16 (141), B16 (142), B16 (143),
+  B16 (144), B16 (145), B16 (146), B16 (147),
+  B16 (148), B16 (149), B16 (150), B16 (151),
+  B16 (152), B16 (153), B16 (154), B16 (155),
+  B16 (156), B16 (157), B16 (158), B16 (159),
+  B16 (160), B16 (161), B16 (162), B16 (163),
+  B16 (132), B16 (165), B16 (166), B16 (167),
+  B16 (168), B16 (169), B16 (170), B16 (171),
+  B16 (172), B16 (173), B16 (174), B16 (175),
+  B16 (176), B16 (177), B16 (178), B16 (179),
+  B16 (180), B16 (181), B16 (182), B16 (183),
+  B16 (184), B16 (185), B16 (186), B16 (187),
+  B16 (188), B16 (189), B16 (190), B16 (191),
+  B16 (192), B16 (193), B16 (194), B16 (195),
+  B16 (196), B16 (197), B16 (198), B16 (199),
+  B16 (200), B16 (201), B16 (202), B16 (203),
+  B16 (204), B16 (205), B16 (206), B16 (207),
+  B16 (208), B16 (209), B16 (210), B16 (211),
+  B16 (212), B16 (213), B16 (214), B16 (215),
+  B16 (216), B16 (217), B16 (218), B16 (219),
+  B16 (220), B16 (221), B16 (222), B16 (223),
+  B16 (224), B16 (225), B16 (226), B16 (227),
+  B16 (228), B16 (229), B16 (230), B16 (231),
+  B16 (232), B16 (233), B16 (234), B16 (235),
+  B16 (236), B16 (237), B16 (238), B16 (239),
+  B16 (240), B16 (241), B16 (242), B16 (243),
+  B16 (244), B16 (245), B16 (246), B16 (247),
+  B16 (248), B16 (249), B16 (250), B16 (251),
+  B16 (252), B16 (253), B16 (254), B16 (255)
+};
 
 static bool
 isubase16 (unsigned char ch)
 {
-  return isxdigit (ch);
+  return ch < sizeof base16_to_int && 0 <= base16_to_int[ch];
 }
 
 static int
@@ -550,7 +644,7 @@ static void
 base16_decode_ctx_init (struct base_decode_context *ctx)
 {
   init_inbuf (ctx);
-  ctx->ctx.base16.have_nibble = false;
+  ctx->ctx.base16.nibble = -1;
   ctx->i = 1;
 }
 
@@ -561,44 +655,42 @@ base16_decode_ctx (struct base_decode_context *ctx,
                    char *restrict out, idx_t *outlen)
 {
   bool ignore_lines = true;  /* for now, always ignore them */
-
-  *outlen = 0;
+  char *out0 = out;
+  signed char nibble = ctx->ctx.base16.nibble;
 
   /* inlen==0 is request to flush output.
      if there is a dangling high nibble - we are missing the low nibble,
      so return false - indicating an invalid input.  */
   if (inlen == 0)
-    return !ctx->ctx.base16.have_nibble;
+    {
+      *outlen = 0;
+      return nibble < 0;
+    }
 
   while (inlen--)
     {
-      if (ignore_lines && *in == '\n')
+      unsigned char c = *in++;
+      if (ignore_lines && c == '\n')
+        continue;
+
+      if (sizeof base16_to_int <= c || base16_to_int[c] < 0)
         {
-          ++in;
-          continue;
+          *outlen = out - out0;
+          return false; /* garbage - return false */
         }
 
-      int nib = c_toupper (*in++);
-      if ('0' <= nib && nib <= '9')
-        nib -= '0';
-      else if ('A' <= nib && nib <= 'F')
-        nib -= 'A' - 10;
+      if (nibble < 0)
+        nibble = base16_to_int[c];
       else
-        return false; /* garbage - return false */
-
-      if (ctx->ctx.base16.have_nibble)
         {
           /* have both nibbles, write octet */
-          *out++ = (ctx->ctx.base16.nibble << 4) + nib;
-          ++(*outlen);
+          *out++ = (nibble << 4) + base16_to_int[c];
+          nibble = -1;
         }
-      else
-        {
-          /* Store higher nibble until next one arrives */
-          ctx->ctx.base16.nibble = nib;
-        }
-      ctx->ctx.base16.have_nibble = !ctx->ctx.base16.have_nibble;
     }
+
+  ctx->ctx.base16.nibble = nibble;
+  *outlen = out - out0;
   return true;
 }
 
-- 
2.41.0