1433636 - Crash in OOM | large | NS_ABORT_OOM | mozilla::safebrowsing::`anonymous namespace'::ReadValue<T>

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-30e684b7-fc2d-404c-9db7-3bad40180126.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:620
1 xul.dll mozilla::safebrowsing::`anonymous namespace'::ReadValue<nsTSubstring<char> > toolkit/components/url-classifier/LookupCacheV4.cpp:472
2 xul.dll mozilla::safebrowsing::LookupCacheV4::LoadMetadata toolkit/components/url-classifier/LookupCacheV4.cpp:544
3 xul.dll mozilla::safebrowsing::LookupCacheV4::LoadFromFile toolkit/components/url-classifier/LookupCacheV4.cpp:169
4 xul.dll mozilla::safebrowsing::LookupCache::LoadPrefixSet toolkit/components/url-classifier/LookupCache.cpp:478
5 xul.dll mozilla::safebrowsing::LookupCache::Open toolkit/components/url-classifier/LookupCache.cpp:83
6 xul.dll mozilla::safebrowsing::Classifier::GetLookupCache toolkit/components/url-classifier/Classifier.cpp:1467
7 xul.dll mozilla::safebrowsing::Classifier::RegenActiveTables toolkit/components/url-classifier/Classifier.cpp:927
8 xul.dll mozilla::safebrowsing::Classifier::Open toolkit/components/url-classifier/Classifier.cpp:262
9 xul.dll nsUrlClassifierDBServiceWorker::OpenDb toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:974

=============================================================

out of memory crashes at startup with this signature show up since firefox 58 on 32-bit and 64-bit builds on windows and they show particularly large memory allocation sizes. over all the crash volume is quite low though...

Comment 1

[François Marier [:francois]]

Attachment: Bug 1433636 - Put a limit on the length of Safe Browsing metadata values.

Disk corruption can lead to the stored length of a value to be
unreasonably large and trigger an OOM.

Since values are all currently <= 32 bytes, we can safely enforce
a 256-byte upper bound.

Review commit: https://reviewboard.mozilla.org/r/217038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217038/

Comment 2

[François Marier [:francois]]

To test my patch, I followed these steps:

1. Create a new browser profile and start Firefox.
2. Trigger a google4 update using about:url-classifier.
3. Try the phishing and malware test page on http://testsafebrowsing.appspot.com.
4. Close Firefox.
5. Go into the cache directory and edit safebrowsing/google4/goog-phish-proto.metadata.
6. Append the string "AAAA" at the start of the file and save.
7. Start Firefox again.
8. Clear the browser cache.
9. Try the phishing and malware test page on http://testsafebrowsing.appspot.com.
10. Trigger a google4 update using about:url-classifier.
11. Clear the browser cache.
12. Try the phishing and malware test page on http://testsafebrowsing.appspot.com.

Results (as expected):

- Test pages are correctly blocked at steps 3 and 12.
- Test pages are not blocked at step 10.

In other words, loading a single failed table from disk will cause the whole google4 database to be blown away. The next update will start from a blank state and work fine.

I also confirmed that without my patch, the same steps would crash the browser at startup.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8947322 [details]
Bug 1433636 - Put a limit on the length of Safe Browsing metadata values.

https://reviewboard.mozilla.org/r/217038/#review223220

If the real limit is 32, why not enforce that instead? If you read 33 now, you know the file is corrupted?

Comment 4

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> If the real limit is 32, why not enforce that instead? If you read 33 now,
> you know the file is corrupted?

That's not actually a limit, but rather what the value currently is. It's the opaque "state" string and its size is not specified. I figured I'd use something much larger in case Google decides to use longer strings in the future.

Comment 5

[François Marier [:francois]]

Comment on attachment 8947322 [details]
Bug 1433636 - Put a limit on the length of Safe Browsing metadata values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217038/diff/1-2/

Comment 6

[François Marier [:francois]]

Attachment: data review request

I added a new telemetry probe to keep an eye on how often this condition occurs and to detect any unusual increases in this kind of failure.

Comment 7

[Robert (rrayborn, he/him)]

Comment on attachment 8947979 [details]
data review request

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate? (see [here](https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_dictionary.md), [here](https://github.com/mozilla-mobile/focus/wiki/Install-and-event-tracking-with-the-Adjust-SDK), and [here](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/index.html) for examples).  Refer to the appendix for "documentation" if more detail about documentation standards is needed.

Yes, in Telemetry declarations in code

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, Telemetry

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes.  This is needed to note when our max length assumption fails.  It appears that may happen in the result of regular corruption or if the 3rd party data source changes.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1

5) Is the data collection request for default-on or default-off?

Default-on

6) Does the instrumentation include the addition of **any *new* identifiers**?

No

7) Is the data collection covered by the existing Firefox privacy notice? 

Yes

8) Does there need to be a check-in in the future to determine whether to renew the data?

No

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55dddaa9b77c
Put a limit on the length of Safe Browsing metadata values. r=gcp

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/55dddaa9b77c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 11

[François Marier [:francois]]

Comment on attachment 8947322 [details]
Bug 1433636 - Put a limit on the length of Safe Browsing metadata values.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1433636
[User impact if declined]: If the Safe Browsing files get corrupted on disk, the browser can crash at startup.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, manually by me.
[Needs manual test from QE? If yes, steps to reproduce]: Probably not needed but instructions are at https://bugzilla.mozilla.org/show_bug.cgi?id=1433636#c2
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's only really adding a single early return when we read bogus value from disk.
[String changes made/needed]: None

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8947322 [details]
Bug 1433636 - Put a limit on the length of Safe Browsing metadata values.

Not a high volume crash but it's nice that we can prevent it. 
Let's uplift this for 59 beta 12.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45c1f99b1b86