Bug 190947 – Post too much text to iFrame could crash webkit

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kyle]

Created attachment 353164 [details]
bug demo

Sending too much data to iFrame could crash webkit on all iOS.

Reproducation Steps:

1. A form with the field which is assigned much text (~250KB).
2. Set the target of the form to an iframe
3. Submit the form

Reproduction page: http://adeline.cc/fe/test/post-to-iframe.html

HD Demo Videos: https://1drv.ms/f/s!Aq4mpP6jpjzKjqYdjanmchdTycD_HA

[Alexey Proskuryakov]

I can reproduce this.

It appears to be a jetsam, and it's quite curious indeed. I wonder if it's actually about laying out this text, not form submission.

[Radar WebKit Bug Importer]

<rdar://problem/45678231>

[Chris Dumez]

Looking at a trace of the WebContent process shortly before the crash we can see it spends a lot of time under:
Sample Count, Samples %, CPU %, Symbol
936, 21.8%, 3.4%, WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) (in WebCore)
936, 21.8%, 3.4%,     WebCore::XSSAuditor::init(WebCore::Document*, WebCore::XSSAuditorDelegate*) (in WebCore)
606, 14.1%, 2.2%,         WebCore::SuffixTree<WebCore::ASCIICodebook>::build(WTF::String const&) (in WebCore)

If I disable XSSAuditor (which is a needed for security) via WebPreferences, the example in question no longer jetsams.

I therefore believe it is caused by the XSSAuditor somehow.

I have no reason to believe this is a reason regression.

[Chris Dumez]

Relevant code:
        RefPtr<FormData> httpBody = documentLoader->originalRequest().httpBody();
        if (httpBody && !httpBody->isEmpty()) {
            httpBodyAsString = httpBody->flattenToString();
            if (!httpBodyAsString.isEmpty()) {
                m_decodedHTTPBody = canonicalize(httpBodyAsString, TruncationStyle::None);
                if (m_decodedHTTPBody.find(isRequiredForInjection) == notFound)
                    m_decodedHTTPBody = String();
                if (m_decodedHTTPBody.length() >= minimumLengthForSuffixTree)
                    m_decodedHTTPBodySuffixTree = std::make_unique<SuffixTree<ASCIICodebook>>(m_decodedHTTPBody, suffixTreeDepth);
            }
        }

[Chris Dumez]

Adding Daniel Bates in cc as he is likely more familiar with XSS auditing code.

[Chris Dumez]

m_decodedHTTPBodySuffixTree seems to be used as an optimization inside XSSAuditor::isContainedInRequest(). If m_decodedHTTPBodySuffixTree is not initialized then we end up doing the search like so:
m_decodedHTTPBody.containsIgnoringASCIICase(decodedSnippet)

I have verified that if we do not initialize m_decodedHTTPBodySuffixTree then its does not jetsam.

So the issue seems to be that m_decodedHTTPBodySuffixTree may end up being extremely big and cause jetsams (not to mention that constructing it can be very slow).

[Chris Dumez]

I will upload a patch shortly.

[Chris Dumez]

Created attachment 354028 [details]
Patch

[Geoffrey Garen]

Comment on attachment 354028 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 354028 [details]
Patch

Clearing flags on attachment: 354028

Committed r237909: <https://trac.webkit.org/changeset/237909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Could this change have a performance impact? It's in the range where we have a 1-1.5% speedometer 2 regression on iOS. It seems like the most likely candidate just by glancing at this range:

https://trac.webkit.org/log/webkit/trunk?verbose=on&rev=237910&stop_rev=237896

[Chris Dumez]

(In reply to Saam Barati from comment #12)
> Could this change have a performance impact? It's in the range where we have
> a 1-1.5% speedometer 2 regression on iOS. It seems like the most likely
> candidate just by glancing at this range:
> 
> https://trac.webkit.org/log/webkit/
> trunk?verbose=on&rev=237910&stop_rev=237896

It could have a performance impact, yes. I am a bit surprised Speedometer is submitting  forms though.

[Chris Dumez]

(In reply to Chris Dumez from comment #13)
> (In reply to Saam Barati from comment #12)
> > Could this change have a performance impact? It's in the range where we have
> > a 1-1.5% speedometer 2 regression on iOS. It seems like the most likely
> > candidate just by glancing at this range:
> > 
> > https://trac.webkit.org/log/webkit/
> > trunk?verbose=on&rev=237910&stop_rev=237896
> 
> It could have a performance impact, yes. I am a bit surprised Speedometer is
> submitting  forms though.

I have just run Speedometer 2.0 locally and did not see it build any SuffixTree. I would therefore be surprised if the regression was caused by this change.

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to Chris Dumez from comment #13)
> > (In reply to Saam Barati from comment #12)
> > > Could this change have a performance impact? It's in the range where we have
> > > a 1-1.5% speedometer 2 regression on iOS. It seems like the most likely
> > > candidate just by glancing at this range:
> > > 
> > > https://trac.webkit.org/log/webkit/
> > > trunk?verbose=on&rev=237910&stop_rev=237896
> > 
> > It could have a performance impact, yes. I am a bit surprised Speedometer is
> > submitting  forms though.
> 
> I have just run Speedometer 2.0 locally and did not see it build any
> SuffixTree. I would therefore be surprised if the regression was caused by
> this change.

Ditto for Speedometer 1.0

[Chris Dumez]

(In reply to Chris Dumez from comment #15)
> (In reply to Chris Dumez from comment #14)
> > (In reply to Chris Dumez from comment #13)
> > > (In reply to Saam Barati from comment #12)
> > > > Could this change have a performance impact? It's in the range where we have
> > > > a 1-1.5% speedometer 2 regression on iOS. It seems like the most likely
> > > > candidate just by glancing at this range:
> > > > 
> > > > https://trac.webkit.org/log/webkit/
> > > > trunk?verbose=on&rev=237910&stop_rev=237896
> > > 
> > > It could have a performance impact, yes. I am a bit surprised Speedometer is
> > > submitting  forms though.
> > 
> > I have just run Speedometer 2.0 locally and did not see it build any
> > SuffixTree. I would therefore be surprised if the regression was caused by
> > this change.
> 
> Ditto for Speedometer 1.0

Besides my change, https://trac.webkit.org/changeset/237903/webkit could also have perf impact I believe in the range you provided.

[Saam Barati]

(In reply to Chris Dumez from comment #16)
> (In reply to Chris Dumez from comment #15)
> > (In reply to Chris Dumez from comment #14)
> > > (In reply to Chris Dumez from comment #13)
> > > > (In reply to Saam Barati from comment #12)
> > > > > Could this change have a performance impact? It's in the range where we have
> > > > > a 1-1.5% speedometer 2 regression on iOS. It seems like the most likely
> > > > > candidate just by glancing at this range:
> > > > > 
> > > > > https://trac.webkit.org/log/webkit/
> > > > > trunk?verbose=on&rev=237910&stop_rev=237896
> > > > 
> > > > It could have a performance impact, yes. I am a bit surprised Speedometer is
> > > > submitting  forms though.
> > > 
> > > I have just run Speedometer 2.0 locally and did not see it build any
> > > SuffixTree. I would therefore be surprised if the regression was caused by
> > > this change.
> > 
> > Ditto for Speedometer 1.0
> 
> Besides my change, https://trac.webkit.org/changeset/237903/webkit could
> also have perf impact I believe in the range you provided.

Thanks for looking into this Chris. I'll post on that bug.