853823 - updater should be case insensitive when parsing hash function in update xml

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[[e-mail] (:bhearsum)]

Attachment: stop carrying about case in hash function

Toolkit's nsUpdateService treats hash functions as case-insensitive (https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3355). Firefox for Android's probably should, too. I ended up discovering this when trying to change update snippets in bug 853036 to help more easily test the new update server.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 728178 [details] [diff] [review]
stop carrying about case in hash function


>-        if ("sha512".equals(hashFunction)) {
>+        if ((hashFunction.compareToIgnoreCase("sha512")) == 0) {
>             javaHashFunction = "SHA-512";
>         } else {
>             Log.e(LOGTAG, "Unhandled hash function: " + hashFunction);

          if (("sha512".compareToIgnoreCase(hashFunction)) == 0) {

hashFunction can be null, so just use the literal string as the LHS

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Finkle (:mfinkle) from comment #1)
>           if (("sha512".compareToIgnoreCase(hashFunction)) == 0) {
> 
> hashFunction can be null, so just use the literal string as the LHS

Unlike equals(), compareToIgnoreCase() throws an NPE if the argument passed in is null, so even using the literal string as the LHS doesn't solve the problem. From the docs it looks like "sha512".equalsIgnoreCase(hashFunction) should be safe but it's probably worth testing first.

Comment 3

[[e-mail] (:bhearsum)]

Attachment: use equalsIgnoreCase

Thanks for the catches. I just verified this:
➜  tmp  cat java.java
public class java {
    public static void main(String[] args) {
        if ("sha512".equalsIgnoreCase("SHA512")) {
            System.out.println("first");
        }
        if ("sha512".equalsIgnoreCase(null)) {
            System.out.println("second");
        }
        if ("sha512".compareToIgnoreCase(null) == 0) {
            System.out.println("third");
        }
    }
}
➜  tmp  java java    
first
Exception in thread "main" java.lang.NullPointerException
	at java.lang.String$CaseInsensitiveComparator.compare(String.java:1177)
	at java.lang.String$CaseInsensitiveComparator.compare(String.java:1170)
	at java.lang.String.compareToIgnoreCase(String.java:1220)
	at java.main(java.java:9)

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 728934 [details] [diff] [review]
use equalsIgnoreCase

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

Thanks!

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 728934 [details] [diff] [review]
use equalsIgnoreCase

No problem. I pushed this to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c457bd84dc66

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Usually whoever merges the patch to m-c will mark the bug fixed.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c457bd84dc66