Re: [bug-gnulib] Re: base64

[Paul Eggert]

Simon Josefsson <address@hidden> writes:


> +  const char *b64 =
> +    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

This should be 'char const b64[64] = "...";' to avoid an unnecessary
indirection and an unneeded byte at the end.  (I prefer putting
"const" after the type that it modifies, as that's a more-consistent
rule, but I realize that tastes differ in this area.)

> +  const unsigned char *iptr = (const unsigned char *) in;

I believe this relies on C99 semantics; if memory serves, C89 doesn't
guarantee that you can cast char * to unsigned char * reliably.
Generally, instead of writing this:

   unsigned char *pu = (unsigned char *) p;
   return *pu;

I prefer to write this:

   return to_uchar (*p);

where to_uchar is defined as follows:

   static inline unsigned char to_uchar (char ch) { return ch; }

This is safe even in C89 and avoids all casts.  (This assumes "inline"
is autoconfed.)

> +  unsigned char *optr = (unsigned char *) out;

I see no need for optr to be unsigned char *.  Why not make it char *?

> +      *optr++ = inlen ? b64[((iptr[1] << 2) +
> +                          (--inlen ? (iptr[2] >> 6) : 0)) & 0x3f] : '=';

This should be indented better, e.g.:

      *optr++ =
        (inlen
         ? b64[((iptr[1] << 2) + (--inlen ? (iptr[2] >> 6) : 0)) & 0x3f]
         : '=');

> +  static const signed char b64[0x100] = {
> +    -1, -1, -1, -1, -1, -1, -1, -1, -1, -2, -2, -1, -1, -2, -1, -1,
> +    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> +    -2, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 62, -1, -1, -1, 63,
> +    52, 53, 54, 55, 56, 57, 58, 59, 60, 61, -1, -1, -1, -3, -1, -1,

I guesss this table assumes ASCII.  Do you want your code to be
portable to EBCDIC hosts?

> +  *outlen = 3 * inlen / 4;   /* FIXME: May allocate one 1 or 2 bytes too
> +                                much, depending on input. */

This can overflow.  You should check for that possibility and return
false when it does.

> +#define BASE64_LENGTH(inlen) (((4 - (inlen) % 3) % 4) + (4 * (inlen) / 3))

As Bruno mentions, this needlessly overflows.  Once you fix that, you
should also fix base64_encode_alloc to fail reliably whenever integer
overflow occurs when it's trying to compute the size (including the +1
at the end).