Re: RFC: cksum --base64/-b support

[Pádraig Brady]

On 31/01/2023 06:48, Jim Meyering wrote:
> On Mon, Jan 30, 2023 at 11:29 AM Pádraig Brady <P@draigbrady.com> wrote:
> > ...
>
> Thanks for the speedy feedback.
>
> > "If must be followed by white space." comment has a typo
> > and also not enforced explicitly, so could be removed.
>
> Thanks. Removed.
>
> > valid_digits() may check beyond the end of the buffer
> > in the case len != BASE64_LENGTH.
>
> Please share how that could happen.
> I don't see how there could be a read-overrun there, since the input
> string is always NUL-terminated.

Oh right I missed that.
Indeed the !isxdigit() would have terminated upon reaching the NUL.
Thanks for using the len though as it's easier to read at least.

Similarly I now see that the `while (!ISWHITE())` can write past the NUL 
terminator,
which could be triggered with a line full of spaces for example.
Patch suggested below...

> However, I've taken your suggestion below:
>
> > Perhaps change the "else" to "else if (len == digest_hex_bytes)".
> > Similarly it may be more defensive / efficient to pass
> > digest_len out of split_3().
>
> I'd considered whether to make *split_3 return that length to the
> caller, but erred on the side of keeping changes small.
> However, given your feedback, I've made both bsd_split_3 and split_3
> return digest length via *D_LEN, and thus avoid adding that strlen
> call.
>
> > non padded base64 encodings are not supported.
> > Is that OK. Worth mentioning in texinfo if so.
>
> IMHO, we must reject a digest that's had any change to its trailing
> "=" (padding) bytes.
> Done.
>
> > A non padded negative test in cksum-c.sh would be useful.
>
> Good idea.
> I've added tests like that in the new test file, removing one or both
> "=" bytes in each of those examples.
>
> V2 attached.

We should also mention that --check autodetects the encoding.
I wonder should we have the separate *sum utils autodetect the encoding also,
but not support the --base64 option. Then `cksum -a blah -b` format
would be supported by `blahsum -c`. Perhaps it's best to restrict to cksum,
to aid in the deprecation of the separate utils.

So suggested changes...

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3624a7d03..f70c3d530 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -4184,6 +4184,11 @@ For the @command{cksum} command, the @option{--check} 
option
 supports auto-detecting the digest algorithm to use,
 when presented with checksum information in the @option{--tag} output format.

+Also for the @command{cksum} command, the @option{--check} option
+supports auto-detecting the digest encoding to use,
+when presented with standard hexidecimal checksums or those generated
+with the @command{cksum} @option{--base64} option.
+
 Output with @option{--zero} enabled is not supported by @option{--check}.
 @sp 1
 For each such line, @command{md5sum} reads the named file and computes its
diff --git a/src/digest.c b/src/digest.c
index 1f98ea7bc..c0616fcb2 100644
--- a/src/digest.c
+++ b/src/digest.c
@@ -646,6 +646,9 @@ valid_digits (unsigned char const *s, size_t len)
           ++s;
         }
     }
+  else
+    return false;
+
   return *s == '\0';
 }

@@ -847,7 +850,7 @@ split_3 (char *s, size_t s_len,

   /* This field must be the hexadecimal or base64 representation
      of the message digest.  */
-  while (!ISWHITE (s[i]))
+  while (s[i] && !ISWHITE (s[i]))
     i++;

   *d_len = &s[i] - (char *) *digest;