1435994 - Crash in nsTSubstring<T>::Replace

Status: RESOLVED WORKSFORME

(Core :: DOM: Security, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-f1e4018a-b966-458a-a2a8-415850180202.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsTSubstring<char>::Replace xpcom/string/nsTSubstring.cpp:650
1 xul.dll nsBase64Encoder::Write netwerk/base/nsBase64Encoder.cpp:27
2 xul.dll nsBinaryOutputStream::WriteFully xpcom/io/nsBinaryStream.cpp:133
3 xul.dll nsBinaryOutputStream::WriteBoolean xpcom/io/nsBinaryStream.cpp:157
4 xul.dll NS_WriteOptionalCompoundObject dist/include/nsIObjectOutputStream.h:118
5 xul.dll nsCSPContext::Write dom/security/nsCSPContext.cpp:1619
6 xul.dll nsBinaryOutputStream::WriteCompoundObject xpcom/io/nsBinaryStream.cpp:352
7 xul.dll NS_WriteOptionalCompoundObject dist/include/nsIObjectOutputStream.h:120
8 xul.dll ContentPrincipal::Write caps/ContentPrincipal.cpp:523
9 xul.dll nsBinaryOutputStream::WriteCompoundObject xpcom/io/nsBinaryStream.cpp:352

=============================================================

these crash reports with an infinite recursion in the stack are spiking up in volume since 2018-02-01.
the increase in crashes seems to come from brazilian users with comments often pointing towards https://email.bol.uol.com.br/login as source of the crashes.

Comment 1

[Nathan Froyd [:froydnj]]

This looks like we're crashing in some CSP code; the topmost interesting frame is:

89 	xul.dll 	ContentPrincipal::Write(nsIObjectOutputStream*) 	caps/ContentPrincipal.cpp:523

which is:

https://hg.mozilla.org/releases/mozilla-release/annotate/c2db4a50dc5c/caps/ContentPrincipal.cpp#l523

  rv = NS_WriteOptionalCompoundObject(aStream, mCSP,
                                      NS_GET_IID(nsIContentSecurityPolicy),
                                      true);

and interspersed in the crashing frames is:

86 	xul.dll 	nsCSPContext::Write(nsIObjectOutputStream*) 	dom/security/nsCSPContext.cpp:1619

which is:

https://hg.mozilla.org/releases/mozilla-release/annotate/c2db4a50dc5c/dom/security/nsCSPContext.cpp#l1619

  nsresult rv = NS_WriteOptionalCompoundObject(aStream,
                                               mSelfURI,
                                               NS_GET_IID(nsIURI),
                                               true);

and also:

83 	xul.dll 	nsHostObjectURI::Write(nsIObjectOutputStream*) 	dom/file/nsHostObjectURI.cpp:95

which is:

  return NS_WriteOptionalCompoundObject(aStream, mPrincipal,
                                        NS_GET_IID(nsIPrincipal),
                                        true);

So that all looks like a big circular mess.  But none of this code is particularly new to 58; a lot of it has been around since FF 49 or even far before that.  ni? to a couple people who might have better ideas about who to talk to and/or what might have changed in the 58 timeframe to cause problems here.  (It's possible the website in question is doing something screwy which messes with our assumptions, too...)

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Christoph would know more about this than I would.

Comment 3

[Kris Maglione [:kmag]]

It looks like we have a blob: URI object with a principal that has a CSP, and the CSP's self URI points back to the original URI object.

I don't know what might have changed to trigger that. If I had to guess, I'd say the circular reference issue isn't new, but we weren't previously trying to send the problematic blob: URIs across processes as often.

Comment 4

[Kris Maglione [:kmag]]

Although... that is a bit odd. The self URI in the CSP for a blob: URI's principal shouldn't be a blob: URI. It should be the URI of the principal it inherits. I wonder if we're attaching a CSP from a blob: document to a principal that it inherited from a non-blob: document... There are probably security implications to that.

Either way, I suppose bug 965637 should fix this.

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #3)
> It looks like we have a blob: URI object with a principal that has a CSP,
> and the CSP's self URI points back to the original URI object.

That is very likely. For the loadingContext and loadingPrincipal we use weakPtrs and I think we haven't really thought about blobs for that. I'll take a closer look at this bug in the next days.

Comment 6

[[:philipp]]

adding two crash signatures that look related.

Comment 7

[[:philipp]]

and the one from fennec...

Comment 8

[Paul Bone [:pbone]]

What has probably changed is the canary in nsStringBuffer introduced in bug 1410276 in an effort to hunt down other problems.

Comment 9

[Paul Bone [:pbone]]

Oh, I may be wrong.  The canary crashes appear to be a small-ish subset of this crash.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This looks pretty frequent on release 58. We've only got a week left in the 59 cycle before the first RC build, but it would be great if we could come up with some sort of mitigation before then.

Comment 11

[[:philipp]]

the crash volume of this has dropped on 2018-02-24 - probably there were changes made on the brazilian webmail site to fix this crashing issue.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

We are pretty unlikely to fix this before 59 release. Looks like the issue improved dramatically with 59 beta 12, so that's encouraging.

Comment 13

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> We are pretty unlikely to fix this before 59 release. Looks like the issue
> improved dramatically with 59 beta 12, so that's encouraging.

Hey Liz,
the issue here improved and I don't think we will be able to fix that for 59, but I am still getting the |Monday Mar 12 -- Daily Release Tracking Alert| email. Can you update flags accordingly please?

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, it looks much better! I'll untrack. I think I will also follow up to ask that we don't send tracking alert messages for "fix-optional" issues.

Comment 15

[Kris Maglione [:kmag]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Yes, it looks much better! I'll untrack. I think I will also follow up to
> ask that we don't send tracking alert messages for "fix-optional" issues.

FWIW, I'd like to still receive tracking alerts for fix-optional issues. They may not be blockers, but I'd generally still like to have them on my radar in case I happen to have the bandwidth to fix them in time...

Comment 16

[Christoph Kerschbaumer [:ckerschb}]

I am closing out old bugs and I think the problem reported here was fixed by bug 965637, hence marking as WORKSFORME.