Crash in nsNavBookmarks::OnVisit by setting long location.hash

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
Places
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cgvwzq, Assigned: mak)

Tracking

({crash, regression, reproducible})

39 Branch
mozilla47
crash, regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160105164030

Steps to reproduce:

location.hash=new Array(10000000).join("A")


Actual results:

Firefox crashed


Expected results:

Give an error for invalid URL.

Comment 1

2 years ago
CR FF46:
https://crash-stats.mozilla.com/report/index/e2256adf-0acd-4982-8f81-0903e2160115

It didn't crash in the past, even if the browser was hanging during generating the array but the console was able to display the result.

reg range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd8e079d6335&tochange=58c9d079f318

Reporter, do you have a crash report ID in about:crashes?
Crash Signature: [@ nsNavBookmarks::OnVisit ]
Flags: needinfo?(pvtolkien)
Summary: Crash by setting loooong location.hash → Crash in nsNavBookmarks::OnVisit by setting long location.hash

Updated

2 years ago
Severity: normal → critical
Keywords: crash, reproducible
Version: 43 Branch → 39 Branch
(Reporter)

Comment 2

2 years ago
Sorry for the delay, I was AFK.

(In reply to Loic from comment #1)
> CR FF46:
> https://crash-stats.mozilla.com/report/index/e2256adf-0acd-4982-8f81-
> 0903e2160115
> 
> It didn't crash in the past, even if the browser was hanging during
> generating the array but the console was able to display the result.
> 
You can avoid the output from console adding a ",1":

location.hash=new Array(10000000).join("A"),1

> reg range:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=fd8e079d6335&tochange=58c9d079f318
> 
> Reporter, do you have a crash report ID in about:crashes?

Sure: https://crash-stats.mozilla.com/report/index/076adbe3-6ae3-4396-904e-516952160118

I could test for Firefox and Firefox Nightly on Linux 64bits and OSX, and all of them seem to crash.
Flags: needinfo?(pvtolkien)
Seems the crash reports related to nsNavBookmarks::OnVisit are in a solid number in the last few days. Based on crash report details, assigning issue to
Status: UNCONFIRMED → NEW
Component: Untriaged → XUL
Ever confirmed: true
Product: Firefox → Core

Updated

2 years ago
Component: XUL → Places
Product: Core → Toolkit
(Assignee)

Comment 4

2 years ago
I'll see if I can figure out something
Assignee: nobody → mak77
(Assignee)

Comment 5

2 years ago
So, it's sort-of a regression caused by Bug 1135354.
That bug is doing the right thing, but some consumers are not properly checking the return value of NS_NewURI, that now returns an error if a spec is longer than 1MB.
So we can have a dereference crash if the nsIURI is used without the rv being checked.
Blocks: 1135354
Status: NEW → ASSIGNED
Keywords: regression
(Assignee)

Comment 6

2 years ago
Ok, after a tedious debugging session I think I figured out something.

What I noticed was that some nsIURI was larger than the limit set by Bug 1135354.
I also noticed that often I had other nsIURI with the same content (host#AAAA...) but length 65536.

Looks like in some cases "something" is already limiting uri size to 65536. I don't know what yet, could be xpidl or something else. That may explain why we didn't notice many crashes due to large uris even before Bug 1135354? Btw, this is uninteresting for this bug since these cropped nsIURIs won't crash. I had no luck figuring where the cut happens.

But, the most interesting thing, is when instead we end up with an nsIURI that is larger than the limit set by Bug 1135354. In all those cases the stack had IPC in the middle. So what I found is that when we serialize and deserialize URIs through IPC we call nsStandardURL::Deserialize.
nsStandardURL::Deserialize does this: mSpec = params.spec();
No length check!

So, I guess we have our culprit, we deserialize an uri that is too long, we build a good nsIURI, but if later we extract the spec and rebuild an nsIURI, it will fail.

So, 2 things:

1. We always assumed we could trust that if we got a spec from nsIURI we could rebuild an nsIURI from it. Looks like it's fragile to assume that, so here I will add checks to Places around NS_NewURI wherever we avoided them in the past.

2. nsStandardURL::Deserialize should check length! This is worrisome, I wonder if other methods can bypass the SetSpec check, like if something changes just the ref or the schema with a very long string, looks like we can again end up into this situation? Patrick, you reviewed the change in bug 1135354, what do you think we should do, just add a length check to Deserialize? How can we test that?
Flags: needinfo?(mcmanus)
the length check makes sense to me, but valentin is the defacto owner of the url code..
Flags: needinfo?(mcmanus) → needinfo?(valentin.gosu)
(Assignee)

Comment 8

2 years ago
Created attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Review commit: https://reviewboard.mozilla.org/r/31951/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31951/
Attachment #8711038 - Flags: review?(dteller)
(Assignee)

Comment 9

2 years ago
Created attachment 8711039 [details]
MozReview Request: nsStandardURL::Deserialize should check max spec length. r=valentin

Review commit: https://reviewboard.mozilla.org/r/31953/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31953/
Attachment #8711039 - Flags: review?(valentin.gosu)
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

https://reviewboard.mozilla.org/r/31951/#review28697

Works for me, but I'd appreciate a little more documentation, since there are new silent semi-failures.

::: toolkit/components/places/History.cpp:757
(Diff revision 1)
> -      (void)NS_NewURI(getter_AddRefs(referrerURI), mPlace.referrerSpec);
> +      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(referrerURI), mPlace.referrerSpec)));

If `referrerURI == nullptr`, shouldn't we also call `HandleError`? Otherwise, could you document why we can just drop it?

::: toolkit/components/places/History.cpp:1637
(Diff revision 1)
> +      if (uri) {

So what's the semantics if `uri` is `nullptr`? Could you document it?

::: toolkit/components/places/nsNavBookmarks.cpp:2577
(Diff revision 1)
> +  if (!uri) {

Here, too, since it fails silently in release, could you document this behavior somewhere?

::: toolkit/components/places/nsNavHistory.cpp:569
(Diff revision 1)
> +      if (uri) {

Here, too.
Attachment #8711038 - Flags: review?(dteller) → review+
So, while I think this fix kinda works, it doesn't fix the underlying problem and could lead to this assertion in debug code because we expect deserialization to always work: https://dxr.mozilla.org/mozilla-central/source/ipc/glue/URIUtils.cpp#119

It would be much better if we could address the cause of the problem, which is that nsIURI setters cause the URL to exceed its max length. There's less than a dozen setters which need to be addressed. The ones that call into :SetSpec are already OK.
We need to do something like 

> if (mSpec.Length() + input.Length() - Ref().Length() > net_GetURLMaxLength()) return error
Flags: needinfo?(valentin.gosu)
(Assignee)

Comment 12

2 years ago
(In reply to Valentin Gosu [:valentin] from comment #11)
> It would be much better if we could address the cause of the problem, which
> is that nsIURI setters cause the URL to exceed its max length. There's less
> than a dozen setters which need to be addressed. The ones that call into
> :SetSpec are already OK.

I suppose there's no case where we explicitly create an URIParams and Deserialize it, but we always go through SerializeURI. Cause otherwise we'd still need to add a check to Deserialize. Regardless an adding an assertion there will be nice.

I think I will split the netwerk part to its own bug and start by landing the crash prevention in Places.
Based on Try I may have broken something in Places, so for now I will fix that, then move to the StandardURL part later, unless you wish to attack that in parallel.
(In reply to Marco Bonardo [::mak] from comment #12)
> (In reply to Valentin Gosu [:valentin] from comment #11)
> > It would be much better if we could address the cause of the problem, which
> > is that nsIURI setters cause the URL to exceed its max length. There's less
> > than a dozen setters which need to be addressed. The ones that call into
> > :SetSpec are already OK.
> 
> I suppose there's no case where we explicitly create an URIParams and
> Deserialize it, but we always go through SerializeURI. Cause otherwise we'd
> still need to add a check to Deserialize. Regardless an adding an assertion
> there will be nice.
> 
> I think I will split the netwerk part to its own bug and start by landing
> the crash prevention in Places.

Sounds good.
(Assignee)

Comment 14

2 years ago
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31951/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8711039 - Attachment is obsolete: true
Attachment #8711039 - Flags: review?(valentin.gosu)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31951/diff/2-3/
(Assignee)

Comment 16

2 years ago
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31951/diff/3-4/
(Assignee)

Comment 17

2 years ago
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31951/diff/4-5/
(Assignee)

Updated

2 years ago
Blocks: 1242961
(Assignee)

Comment 18

2 years ago
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31951/diff/5-6/
(Assignee)

Comment 19

2 years ago
Ok, so the Places breakage was due to me using an nsCOMPtr after a .forget() call. Glad we have tests :)

Apart that, I had to convert a couple assertions to warnings, cause some automated tests like to test for invalid URIs that "somehow" entered the Places database, and they were clearly hitting the assertions.

the nsStandardURL fixed will happen in bug 1242961.

Comment 21

2 years ago
As it's a regression since FF39 and FF45 is the next ESR, requesting tracking.
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29b22e412b65
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 23

2 years ago
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Approval Request Comment
[Feature/regressing bug #]: bug 1135354
[User impact if declined]: Crash in rare cases, but with a properly built page it could be used for a DOS
[Describe test coverage new/current, TreeHerder]: the touched code is tested in various automated tests
[Risks and why]: low risk, mostly null checks
[String/UUID change made/needed]: none
Attachment #8711038 - Flags: approval-mozilla-beta?
Attachment #8711038 - Flags: approval-mozilla-aurora?
status-firefox44: affected → wontfix
tracking-firefox45: ? → +
tracking-firefox46: ? → +
tracking-firefox47: ? → +
Comment on attachment 8711038 [details]
MozReview Request: Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric

Fix a crash taking it.
Should be in 45 beta 2.
Attachment #8711038 - Flags: approval-mozilla-beta?
Attachment #8711038 - Flags: approval-mozilla-beta+
Attachment #8711038 - Flags: approval-mozilla-aurora?
Attachment #8711038 - Flags: approval-mozilla-aurora+

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/df73e6201aa5
status-firefox46: affected → fixed

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/10d9a03ded93
status-firefox45: affected → fixed
You need to log in before you can comment on or make changes to this bug.