1777600 - Crash in [@ nsUrlClassifierPrefixSet::GetPrefixesNative]

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/d144b842-2f29-427d-bb2a-983700220625

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsUrlClassifierPrefixSet::GetPrefixesNative toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:200
1 xul.dll mozilla::safebrowsing::VariableLengthPrefixSet::GetFixedLengthPrefixes toolkit/components/url-classifier/VariableLengthPrefixSet.cpp:225
2 xul.dll mozilla::safebrowsing::Classifier::ReadNoiseEntries toolkit/components/url-classifier/Classifier.cpp:1625
3 xul.dll nsUrlClassifierDBServiceWorker::AddNoise toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:459
4 xul.dll nsUrlClassifierDBServiceWorker::HandlePendingLookups toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:431
5 xul.dll UrlClassifierDBServiceWorkerProxy::LookupRunnable::Run toolkit/components/url-classifier/nsUrlClassifierProxies.cpp:36
6 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:465
7 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:330
8 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:373
9 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:355

This crash is odd. It seems like we're crashing while accessing the outArray parameter, but the accesses are guarded against the length we set for the array. The crashing addresses are often on a page boundary, and checking a few minidumps I noticed that they correspond either with the end or the beginning of a mapped region. Not being familiar with this code it's hard for me to tell what's going on.

Comment 1

[Tim Huang[:timhuang]]

Attachment: Bug 1777600 - Part 1: Implement nsUrlClassifierPrefixSet::GetPrefixByIndex(). r?dimi!

This patch implements a nsUrlClassifierPrefixSet::GetPrefixByIndex() that
allows to get the specific prefix with the given index.

Comment 2

[Tim Huang[:timhuang]]

Attachment: Bug 1777600 - Part 2: Add a gTest to verify nsUrlClassifierPrefixSet::GetPrefixByIndex(). r?dimi!

Depends on D161769

Comment 3

[Tim Huang[:timhuang]]

Attachment: Bug 1777600 - Part 3: Get noise prefixes by index instead of the entire prefix set. r?dimi!

In Classifier::ReadNoiseEntries(), we used to get the entire prefix set
in order to get noise prefixes. This patch improves it by getting noise
prefixes by index. So, we don't need the entire prefix set.

The patch introduces new functions in LookupCacheV2, V4 and
VariableLengthPrefixSet to allow getting target prefix by index. Also,
we need to inroduces new functions to get the length of the prefix set
to run the noise pick up algorithm.

Depends on D161770

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0624a8aa7c2
Part 1: Implement nsUrlClassifierPrefixSet::GetPrefixByIndex(). r=dimi
https://hg.mozilla.org/integration/autoland/rev/ccaf8757d382
Part 2: Add a gTest to verify nsUrlClassifierPrefixSet::GetPrefixByIndex(). r=dimi
https://hg.mozilla.org/integration/autoland/rev/be9e99eac13c
Part 3: Get noise prefixes by index instead of the entire prefix set. r=dimi

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c0624a8aa7c2
https://hg.mozilla.org/mozilla-central/rev/ccaf8757d382
https://hg.mozilla.org/mozilla-central/rev/be9e99eac13c

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from mozilla-central for [@ mozilla::safebrowsing::Classifier::ReadNoiseEntries] crashes:
https://hg.mozilla.org/mozilla-central/rev/8495494c57f82b71854b362bfbd40f9998933908

crash-stats reports 590 crashes from 231 installations, e.g. bp-680d8e4a-16bf-48ba-bd15-b07210221115. All with Firefox 109.0a1 20221114214403.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d98e1f48060
Part 1: Implement nsUrlClassifierPrefixSet::GetPrefixByIndex(). r=dimi
https://hg.mozilla.org/integration/autoland/rev/a2ba53e09cf8
Part 2: Add a gTest to verify nsUrlClassifierPrefixSet::GetPrefixByIndex(). r=dimi
https://hg.mozilla.org/integration/autoland/rev/0eabe4cb6889
Part 3: Get noise prefixes by index instead of the entire prefix set. r=dimi

Comment 8

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/4d98e1f48060
https://hg.mozilla.org/mozilla-central/rev/a2ba53e09cf8
https://hg.mozilla.org/mozilla-central/rev/0eabe4cb6889