Re: [Qemu-devel] [PATCH v2 1/5] util: add base64 decoding function

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3310 bytes --]

On 11/24/2015 08:02 AM, Daniel P. Berrange wrote:
> The standard glib provided g_base64_decode doesn't provide any
> kind of sensible error checking on its input. Add a QEMU custom
> wrapper qbase64_decode which can be used with untrustworthy
> input that can contain invalid base64 characters, embedded
> NUL characters, or not be NUL terminated at all.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

> +/**
> + * qbase64_decode:
> + * @input: the (possibly) base64 encoded text
> + * @in_len: length of @input or -1 if NUL terminated
> + * @out_len: filled with length of decoded data
> + * @errpr: pointer to uninitialized error object

s/errpr/errp/

> + * Returns: the decoded data or NULL
> + */
> +uint8_t *qbase64_decode(const char *input,
> +                        size_t in_len,
> +                        size_t *out_len,
> +                        Error **errp);
> +

Is char* any easier to work with than uint8_t* as the return type?  I'm
fine with either, and actually like that uint8_t doesn't cause
unintentional sign-extension on 8-bit input, but just want to make sure
we aren't forcing the majority of our callers to cast back to a more
convenient type.


> +static void test_base64_good(void)
> +{
> +    const char *input = "QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZS"
> +        "BzbmFrZSwgd2UgbWlzc2VkIHRoZSBzY29ycGlvbi4=";
> +    const char *expect = "Because we focused on the snake, "
> +        "we missed the scorpion.";
> +
> +    size_t len;
> +    uint8_t *actual = qbase64_decode(input,
> +                                     -1,
> +                                     &len,
> +                                     &error_abort);
> +
> +    g_assert(actual != NULL);
> +    g_assert_cmpint(len, ==, strlen(expect));
> +    g_assert_cmpstr((char *)actual, ==, expect);
> +    g_free(actual);

For example, this demonstrates a caller having to cast. But it doesn't
show whether more callers want char* or uint8_t*.


> +
> +static void test_base64_embedded_nul(void)
> +{
> +    const char input[] = "There's no such\0thing as a free lunch.";
> +
> +    test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> +}
> +
> +
> +static void test_base64_not_nul_terminated(void)
> +{
> +    char input[] = "There's no such\0thing as a free lunch.";
> +    input[G_N_ELEMENTS(input) - 1] = '!';
> +
> +    test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> +}
> +

Did you mean to have an embedded NUL in the second test, or should you
change that \0 to space?

> +++ b/util/base64.c

> +#include "qemu/base64.h"
> +
> +static const char *base64_valid_chars =
> +    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=";

Do we want to allow newlines (perhaps by adding a bool parameter)?
After all, although newline is not valid in base64, it is the one
character that GNU coreutils special-cases (produce on output every
--wrap columns, ignore on input without needing --ignore-garbage) to
make base64 blocks easier to read by breaking into lines rather than one
long string - and which may be relevant if someone is pasting output
from base64(1) into QMP.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]