Closed Bug 1435329 Opened 3 years ago Closed 3 years ago

nsRFPService.h's KeyboardHashKey::HashKey method is inefficient

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: froydnj, Assigned: tjr)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

Bug 1435329 Improve the HashKey function in the Resist Fingerprinting Component
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
The code reads:

  static PLDHashNumber HashKey(KeyTypePointer aKey)
  {
    nsAutoString temp;
    temp.AppendInt(aKey->mLang);
    temp.Append('|');
    temp.AppendInt(aKey->mRegion);
    temp.Append('|');
    temp.AppendInt(aKey->mKeyIdx);
    temp.Append('|');
    temp.Append(aKey->mKey);
    return mozilla::HashString(temp);
  }

We shouldn't need to build up a temporary string just to calculate its hash; we can calculate the hashes of the individual members and combine them.  mfbt/HashFunctions.h even provides examples of how this can be done.
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Comment on attachment 8952059 [details]
Bug 1435329 Improve the HashKey function in the Resist Fingerprinting Component

https://reviewboard.mozilla.org/r/221278/#review227230

::: commit-message-ce24b:3
(Diff revision 1)
> +While we could cut the order of the inputs over directly to the AddToHash methods
> +(because there is not AddToHash(string) - we could rearrange them and then not need
> +to build a temporary string.

This message reads peculiarly.  Did you mean to say "While we could *not* cut the order..."?

I also think "(because AddToHash(string) does not exist)" reads better.

::: toolkit/components/resistfingerprinting/nsRFPService.h:133
(Diff revision 1)
> -    temp.AppendInt(aKey->mLang);
> -    temp.Append('|');
> -    temp.AppendInt(aKey->mRegion);
> -    temp.Append('|');
> -    temp.AppendInt(aKey->mKeyIdx);
> -    temp.Append('|');
> +    hash = mozilla::AddToHash(hash, "|");
> +    hash = mozilla::AddToHash(hash, aKey->mRegion);
> +    hash = mozilla::AddToHash(hash, "|");
> +    hash = mozilla::AddToHash(hash, aKey->mKeyIdx);
> +    hash = mozilla::AddToHash(hash, "|");
> +    hash = mozilla::AddToHash(hash, aKey->mLang);

I'm curious whether we really want the `"|"` bits in here.

I think this would be slightly nicer as:

```
return AddToHash(hash,
                 aKey->mRegion,
                 aKey->mKeyIdx,
                 aKey->mLang);
```

(with the `"|"` added at appropriate places if we decide we really want those.)

Did that not work well?
Attachment #8952059 - Flags: review?(nfroyd) → review+
Comment on attachment 8952059 [details]
Bug 1435329 Improve the HashKey function in the Resist Fingerprinting Component

https://reviewboard.mozilla.org/r/221278/#review227264

::: toolkit/components/resistfingerprinting/nsRFPService.h:133
(Diff revision 1)
> -    temp.AppendInt(aKey->mLang);
> -    temp.Append('|');
> -    temp.AppendInt(aKey->mRegion);
> -    temp.Append('|');
> -    temp.AppendInt(aKey->mKeyIdx);
> -    temp.Append('|');
> +    hash = mozilla::AddToHash(hash, "|");
> +    hash = mozilla::AddToHash(hash, aKey->mRegion);
> +    hash = mozilla::AddToHash(hash, "|");
> +    hash = mozilla::AddToHash(hash, aKey->mKeyIdx);
> +    hash = mozilla::AddToHash(hash, "|");
> +    hash = mozilla::AddToHash(hash, aKey->mLang);

My initial attempt at putting everything into one AddToHash didn't work, and I didn't try after re-arranging. But this works now.

Normally I prefer seperators - the PGP v3 Key Id Collisions is one of my favorite crypto bugs, and was caused by no distinugishment between the expoonent and modulous. 

But here, the final fields are all fixed-length, so we can safely drop the pipes.
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab3dd0846b7b
Improve the HashKey function in the Resist Fingerprinting Component r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ab3dd0846b7b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.