Bug 53405

Summary:

XSS Auditor is spinning inside decodeURLEscapeSequences() if there are percent signs in large posted data

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

Page Loading

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, dbates

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

49845

Bug Blocks:

Attachments:

Description	Flags
test case	none
Patch	none
Patch	none

Description Alexey Proskuryakov 2011-01-30 15:15:35 PST

We're seeing frequent freezes on an Apple internal web site after submitting a form - usually about half a minute for a small page. This seems to be a different issue than bug 49845 - here we aren't rebuilding the tree, but each check is extremely slow.

Attaching a reduced test case.

<rdar://problem/8227019>

Comment 1 Alexey Proskuryakov 2011-01-30 15:15:56 PST

Created attachment 80603 [details]
test case

Comment 2 Adam Barth 2011-01-30 22:35:07 PST

I suspect this will be fixed at the same time as Bug 49845.

Comment 3 Adam Barth 2011-02-03 01:03:40 PST

The problem here seems to be that decodeURLEscapeSequences is really slow on this input.  It shouldn't be.  I'll investigate some more.

Comment 4 Adam Barth 2011-02-03 10:30:24 PST

I see the bug.  This algorithm is N^2!

Comment 5 Adam Barth 2011-02-03 10:58:51 PST

Created attachment 81086 [details]
Patch

Comment 6 Alexey Proskuryakov 2011-02-03 11:32:08 PST

Comment on attachment 81086 [details]
Patch

Nice!

The logic remains slightly twisted in that the second sequence in "%FF%zz" will be checked twice. It's not important in practice, but makes the code harder to follow. 

-
+        

I don't care personally, but we usually prefer no trailing whitespace.

It's obviously hard to make a regression test for this, but since major URL code rewrite is not off the table, a test would be nice.

Comment 7 Adam Barth 2011-02-03 12:23:03 PST

Created attachment 81097 [details]
Patch

Comment 8 Adam Barth 2011-02-03 12:24:33 PST

> I don't care personally, but we usually prefer no trailing whitespace.

Sorry.  I'm not that facile with the Xcode editor.

> It's obviously hard to make a regression test for this, but since major URL code rewrite is not off the table, a test would be nice.

We do have those "order of magnitude" tests for editing.  I might be possible to adapt those for this case...

Comment 9 Adam Barth 2011-02-03 12:25:41 PST

Comment on attachment 81097 [details]
Patch

Trying to write a test for this sounds like it will cause more trouble than its worth.  This patch is basically a performance optimization, which we typically don't write tests for.

Comment 10 Alexey Proskuryakov 2011-02-03 14:37:39 PST

Well, we have a few tests for algorithmic improvements like this, and I don't think that they've been flaky recently. But this was only a suggestion.

Comment 11 Alexey Proskuryakov 2011-02-03 14:38:45 PST

Thank you for fixing this!

Comment 12 WebKit Commit Bot 2011-02-03 21:27:10 PST

Comment on attachment 81097 [details]
Patch

Clearing flags on attachment: 81097

Committed r77600: <http://trac.webkit.org/changeset/77600>

Comment 13 WebKit Commit Bot 2011-02-03 21:27:16 PST

All reviewed patches have been landed.  Closing bug.