724152 - Honor URL_SAFE flag for base64 encoding/decoding

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: patch

With bug 723550, for Froyo and higher, we use the URL_SAFE flag for all base64 encodings, but we use the DEFAULT flag for decodings. There are a few problems with this:

1) encodeBase64 and decodeBase64 should be inverse functions such that X equals decodeBase64(encodeBase64(X)). This isn't true since we use a different set of flags for each operation.

2) The base64 implementation in GeckoAppShell, used in Eclair, always assumes URL_SAFE. I think this will break search engine icons in Eclair (though I don't have an Eclair device to test this with).

This patch makes the base64 implementation in GeckoAppShell honor the URL_SAFE flag and requires callers to specify which encoding/decoding flags will be used.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: patch

Fixed bug number/description in commit message.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 594353 [details] [diff] [review]
patch

Review of attachment 594353 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoAppShell.java
@@ +1861,5 @@
>          for (int i=0; i<map2.length; i++) map2[i] = -1;
>          for (int i=0; i<64; i++) map2[map1[i]] = (byte)i;
> +        map2_urlsafe = map2.clone();
> +        map2_urlsafe[map1_urlsafe[62]] = (byte)62;
> +        map2_urlsafe[map1_urlsafe[63]] = (byte)63;

there is no need for two maps here

@@ +1919,5 @@
>          byte[] out = new byte[oLen];
>          int ip = iOff;
>          int iEnd = iOff + iLen;
>          int op = 0;
> +        byte[] fromMap = ((flags & BASE64_URL_SAFE) == 0 ? map2 : map2_urlsafe);

just use map2 (with the two additional entries)

@@ +1930,5 @@
>                  throw new IllegalArgumentException ("Illegal character in Base64 encoded data.");
> +            int b0 = fromMap[i0];
> +            int b1 = fromMap[i1];
> +            int b2 = fromMap[i2];
> +            int b3 = fromMap[i3];

again, just map2

Comment 3

[Brian Nicholson (:bnicholson)]

Landed with blassey's changes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/37ce3408d813

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 594353 [details] [diff] [review]
patch

[Approval Request Comment]
Makes base64 API consistent for encoding/decoding and for Eclair/post-Eclair. Low risk.

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/37ce3408d813

Comment 6

[Alex Keybl [:akeybl]]

Comment on attachment 594353 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 7

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/be24af2fb7f6

Comment 8

[Justin Wood (:Callek)]

FYI was going to help clear the queue here:

Justin@ORION /d/sources/mozilla-beta
$ hg qpush
applying base64.patch
patching file mobile/android/base/GeckoAppShell.java
Hunk #1 FAILED at 1828
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/GeckoAppShell.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh base64.patch

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 594353 [details] [diff] [review]
patch

Clearing approval for Aurora 12 and Beta 11 because we are not currently planning a Native Fennec release of these versions.  If this changes in the future, we will likely do a mass uplift of all native fennec changes.  For now, let's get these bugs off the channel triage radar.

[Filter on the string "mbrubeck-bugspam" if you want to delete all of these emails at once.]