Switching list update response for initial download response causes crash

VERIFIED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Toolkit
Safe Browsing
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: mwobensmith, Assigned: francois)

Tracking

44 Branch
mozilla45
Points:
---

Firefox Tracking Flags

(firefox44 verified, firefox45 verified, b2g-v2.5 fixed)

Details

(URL)

Attachments

(3 attachments)

1. Unzip sample files.
2. Launch pyserver13000.py 
3. Launch pyserver13001.py
4. Change Nightly Fx44 prefs for shavar list URL to "localhost:13000"
5. Change "nextupdatetime" pref to 1.
6. Relaunch Nightly via command line, using environment variables from here for debugging: https://wiki.mozilla.org/Phishing_Protection#QA

Result:
Crash - see URL

NOTE:
The fun part here is that although the Python scripts above indicate connections on ports 13000 and 13001 respectively, I simply switched them for this test. What then happens is that the initial query for table names, update interval etc. instead gets an update list response.
Created attachment 8672075 [details]
crash.zip
(Assignee)

Updated

3 years ago
Blocks: 1149867
(Assignee)

Comment 2

3 years ago
Here's the reason for that crash:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffca6ff700 (LWP 367)]
mozilla::safebrowsing::ProtocolParser::ProcessChunkControl (this=0x7fffca7fb320, aLine=...) at /home/francois/devel/mozilla-central/toolkit/components/url-classifier/ProtocolParser.cpp:199
199	  if (!mTableUpdate || mTableUpdate->TableName().IsEmpty()) {
(gdb) p mTableUpdate
$1 = (mozilla::safebrowsing::TableUpdate *) 0xe4e4e4e4e4e4e4e4

De-referencing an uninitialized pointer.
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created attachment 8693145 [details] [diff] [review]
bug1213400.patch
Attachment #8693145 - Flags: review?(gpascutto)
Attachment #8693145 - Flags: review?(gpascutto) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 5

3 years ago
Comment on attachment 8693145 [details] [diff] [review]
bug1213400.patch

Approval Request Comment
[Feature/regressing bug #]: crash on invalid responses from safe browsing / tracking protection servers
[User impact if declined]: Could cause a browser crash if the trusted endpoints we use malfunction for some reason (which prevents firefox from working even in safe mode until the server is fixed).
[Describe test coverage new/current, TreeHerder]: manual testing as per Matt's steps in this bug
[Risks and why]: Prior to this, our null point checks were kind of pointless since the pointer was non-null but pointing to uninitialized memory. So theoretically, this could surface some bugs where we were using an uninitialized pointer without crashing, but it seems unlikely.
[String/UUID change made/needed]: none
Attachment #8693145 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 6

3 years ago
(In reply to François Marier [:francois] from comment #5)
> [User impact if declined]: Could cause a browser crash if the trusted
> endpoints we use malfunction for some reason (which prevents firefox from
> working even in safe mode until the server is fixed).

Correction: safe-mode is not affected by this crash since all of Safe Browsing is disabled in safe-mode (see bug 1134954).

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d5314a40363
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 8

3 years ago
Matt, could you please verify this issue is fixed as expected on latest Nightly? Thanks!
Flags: needinfo?(mwobensmith)

Comment 9

3 years ago
Comment on attachment 8693145 [details] [diff] [review]
bug1213400.patch

Crash fixes are good, Aurora44+
Attachment #8693145 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 10

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c145973b6cb
status-firefox44: affected → fixed
Created attachment 8696034 [details]
aurora_44_crash_2015-12-04.txt

Nightly 45 looks good.

However, I'm still getting a crash on Aurora 44. See attached log. Let me know if you'd like me to do investigate more.
Flags: needinfo?(mwobensmith)
OK, apologies - checkin didn't happen until this morning, so perhaps I jumped the gun. I'll wait until we have a build to try it on Aurora 44. Sorry about that.
Verified on latest builds of both Nightly and Aurora.
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
status-firefox45: fixed → verified
(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #14)
> Verified on latest builds of both Nightly and Aurora.

This is great. Thank you!
You need to log in before you can comment on or make changes to this bug.