Bug 23566 – base64Decode() patch

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Paul Pedriana]

The base64Decode function doesn't follow the Base64 convention regarding unrecognized encoding characters, as stated in RFC 2045:

    All line breaks or other characters not found in 
    Table 1 must be ignored by decoding software.

Similary, there is corresponding code comments in some WebKit code:

    // Use the GLib Base64 if available, since WebCore's decoder isn't
    // general-purpose and fails on Acid3 test 97 (whitespace).

This patch fixes the base64Decode function in this respect.

[Paul Pedriana]

Created attachment 27068 [details]
base64Decode patch

[Darin Adler]

Comment on attachment 27068 [details]
base64Decode patch

Looks good!

> [phone]  Paul  <set EMAIL_ADDRESS environment variable>

This should have your full name and your email address. Remember that prepare-ChangeLog helps you write a change log, but doesn't write it for you. You may have to fix things.

> +          Fixed bug whereby base64Decode() was failing when unrecognized encoding chars

You call this a bug in base64Decode, but it's actually the design of base64Decode. For example, this comment is in the header:

    // this decoder is not general purpose - it returns an error if it encounters a linefeed, as needed for window.atob

Changing this behavior might be helpful for some callers and harmful for others.

> +          were present, whereas the base64 specification requires such chars be ignored.
> +          This allows some base64 tests, such as that in Acid3, to succeed where they previous failed.

I don't understand this. I believe the Acid3 test already passes. Perhaps you're talking specifically about something in the curl or soup handling of data URLs? If so, please be specific.

I believe your change may have improved data URLs for those protocols, but incompatibly changed the behavior of window.atob. Please supply some test cases to demonstrate the change in behavior of atob and check the behavior of other browsers, or submit a patch that does not change the behavior of atob.

Also, please be more specific in your comments. I don't think "some base64 tests, such as that in Acid3" points clearly enough to the curl and soup implementations, if indeed that's what you're fixing.

review- because either:

    1) This fixes a bug in atob, but doesn't include a test case demonstrating that bug.

or

    2) This introduces a bug in atob, so shouldn't be checked in as-is.

[Darin Adler]

This bug needs a title that makes it clear what the problem is. Perhaps "data URL decoding in curl/soup does not skip illegal base64 characters as required by spec"?

[Paul Pedriana]

I posted this as a followup to this email:
    https://lists.webkit.org/pipermail/webkit-dev/2008-December/006015.html

The original bug is that base-64 data URL data decoding in some cases is incorrect, but it seems to me that the existing WebKit window.atob is also incorrect, now that you point it out. 

Acid3 data URL support might pass for CoreFoundation, but it won't pass for other back-ends present in the WebKit trunk.

The window.atob function, by definition, decodes base-64 data (e.g. http://docs.sun.com/source/816-6408-10/window.htm#1201548), and so this patch would fix a bug in the existing window.atob implementation. 

However, as you point out, this patch should include a test case exercising the effect on window.atob. Perhaps I can augment it to do this and resubmit. I'm pretty sure the patch would be safe for window.atob, as it simply allows something to work that would previously fail (where it shouldn't).

Does that seem reasonable, or should the issue of data URLs and window.atob be addressed independently for some reason?

[Anne van Kesteren]

The code probably changed a fair bit here over the years, but we also don't want to follow the RFC directly as stated above. The code should be even clearer about this today and it has better test coverage too.