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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[cgvwzq]

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

[Loic]

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?

Comment 2

[cgvwzq]

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.

Comment 3

[Adrian Florinescu [:aflorinescu]]

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

Comment 4

[Marco Bonardo [:mak]]

I'll see if I can figure out something

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Marco Bonardo [:mak]]

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?

Comment 7

[Patrick McManus [:mcmanus]]

the length check makes sense to me, but valentin is the defacto owner of the url code..

Comment 8

[Marco Bonardo [:mak]]

Attachment: 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/

Comment 9

[Marco Bonardo [:mak]]

Attachment: 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/

Comment 10

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

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

Comment 12

[Marco Bonardo [:mak]]

(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.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 14

[Marco Bonardo [:mak]]

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/

Comment 15

[Marco Bonardo [:mak]]

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/

Comment 16

[Marco Bonardo [:mak]]

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/

Comment 17

[Marco Bonardo [:mak]]

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/

Comment 18

[Marco Bonardo [:mak]]

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/

Comment 19

[Marco Bonardo [:mak]]

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 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/29b22e412b65

Comment 21

[Loic]

As it's a regression since FF39 and FF45 is the next ESR, requesting tracking.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29b22e412b65

Comment 23

[Marco Bonardo [:mak]]

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

Comment 24

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df73e6201aa5

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/10d9a03ded93