Closed Bug 1777600 Opened 2 years ago Closed 1 year ago

Crash in [@ nsUrlClassifierPrefixSet::GetPrefixesNative]

Categories

(Toolkit :: Safe Browsing, defect)

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
109 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox107 --- wontfix
firefox108 --- wontfix
firefox109 --- fixed

People

(Reporter: gsvelto, Assigned: timhuang)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

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.

Assignee: nobody → tihuang
Status: NEW → ASSIGNED

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

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

Pushed by tihuang@mozilla.com:
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

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.

Status: RESOLVED → REOPENED
Flags: needinfo?(tihuang)
Resolution: FIXED → ---
Target Milestone: 109 Branch → ---
Regressions: 1800604
Crash Signature: [@ nsUrlClassifierPrefixSet::GetPrefixesNative] → [@ mozilla::safebrowsing::Classifier::ReadNoiseEntries] [@ nsUrlClassifierPrefixSet::GetPrefixesNative]
Pushed by tihuang@mozilla.com:
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
Flags: needinfo?(tihuang)
Regressions: 1800660
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 109 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: