[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/5] util: add base64 decoding function
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/5] util: add base64 decoding function |
Date: |
Tue, 24 Nov 2015 08:54:24 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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 <address@hidden>
> ---
> +/**
> + * 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
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 0/5] Add framework for passing secrets to QEMU, Daniel P. Berrange, 2015/11/24
- [Qemu-devel] [PATCH v2 2/5] qemu-char: convert to use error checked base64 decode, Daniel P. Berrange, 2015/11/24
- [Qemu-devel] [PATCH v2 1/5] util: add base64 decoding function, Daniel P. Berrange, 2015/11/24
- Re: [Qemu-devel] [PATCH v2 1/5] util: add base64 decoding function,
Eric Blake <=
- [Qemu-devel] [PATCH v2 5/5] crypto: add support for loading encrypted x509 keys, Daniel P. Berrange, 2015/11/24
- [Qemu-devel] [PATCH v2 3/5] qga: convert to use error checked base64 decode, Daniel P. Berrange, 2015/11/24
- [Qemu-devel] [PATCH v2 4/5] crypto: add QCryptoSecret object class for password/key handling, Daniel P. Berrange, 2015/11/24