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

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Dec 08, 2015 at 09:18:30AM -0700, Eric Blake wrote:
> On 11/27/2015 09:30 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
> > + * @errp: pointer to uninitialized error object
> 
> That almost implies that I could do:
> 
> Error *err;
> qbase64_decode(,&err);
> 
> In reality, it should be the NULL-initialized error object, as in:
> 
> Error *err = NULL;
> 
> but I don't know if there is a better way to represent it in text.  At
> any rate, that phrase exists elsewhere, so it would be easier to do a
> tree-wide cleanup if we have a better terminology to use, and I won't
> hold up review on this patch for it.

Yep, most of those comments will be my fault, since that's the standard
doc text I've been adding for @errp parameters. I'll fix this one
and also do a general cleanup patch for others separately.

> > +static void test_base64_bad(const char *input,
> > +                            size_t input_len)
> > +{
> > +    size_t len;
> > +    Error *err = NULL;
> > +    uint8_t *actual = qbase64_decode(input,
> > +                                     input_len,
> > +                                     &len,
> > +                                     &err);
> > +
> > +    g_assert(err != NULL);
> 
> Could use &error_abort in the original call instead of a second check
> for err != NULL; but that's cosmetic.

Actually we can't use &error_abort, as we don't want the test
function to abort on error as that would indicate test failure.
We want to make sure the call completes & returns an error
set without aborting.

> > +    g_assert(actual == NULL);
> > +    g_assert_cmpint(len, ==, 0);
> 
> So you are testing that we initialize output length even on error,
> rather than leaving it uninitialized.  That's fair.

> > +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);
> > +}
> > +
> 
> This asserts that you have a failure, but doesn't say what that failure
> would be...
> 
> > +
> > +static void test_base64_not_nul_terminated(void)
> > +{
> > +    char input[] = "There's no such thing as a free lunch.";
> > +    input[G_N_ELEMENTS(input) - 1] = '!';
> > +
> > +    test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> > +}
> > +
> > +
> > +static void test_base64_invalid_chars(void)
> > +{
> > +    const char *input = "There's no such thing as a free lunch.";
> > +
> > +    test_base64_bad(input, strlen(input));
> > +}
> 
> ...and this same input already fails because it doesn't match base64,
> regardless of whether NUL bytes are mishandled.  I wonder if
> test_base64_embedded_nul() and test_base64_not_nul_terminated() should
> be using variations of your known-good base64 string
> ("QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\n"
> "lzc2VkIHRoZSBzY29ycGlvbi4="), to prove that they are failing purely
> because of the NUL handling and not because of invalid base64 content.

That's a good idea - I'll respin with that an add more comments
about what we're testing for in each case.

> But that's only a weak complaint - because by peering into the black
> box, I see that you are fully testing all code paths (that is, the black
> box checks for NUL abuse prior to checking for valid base64 data), so
> I'm not going to insist on a respin.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|