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

Alexey Proskuryakov

Reported 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>

Attachments
test case (80.84 KB, text/html) 2011-01-30 15:15 PST, Alexey Proskuryakov	no flags	Details
Patch (2.10 KB, patch) 2011-02-03 10:58 PST, Adam Barth	no flags	Details Formatted Diff Diff
Patch (2.05 KB, patch) 2011-02-03 12:23 PST, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

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

Created attachment 80603 [details] test case

Adam Barth

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

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

Adam Barth

Comment 3 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.

Adam Barth

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

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

Adam Barth

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

Created attachment 81086 [details] Patch

Alexey Proskuryakov

Comment 6 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.

Adam Barth

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

Created attachment 81097 [details] Patch

Adam Barth

Comment 8 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...

Adam Barth

Comment 9 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.

Alexey Proskuryakov

Comment 10 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.

Alexey Proskuryakov

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

Thank you for fixing this!

WebKit Commit Bot

Comment 12 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>

WebKit Commit Bot

Comment 13 2011-02-03 21:27:16 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.