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 <berra...@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 :|
