Bug 53405 - XSS Auditor is spinning inside decodeURLEscapeSequences() if there are percent signs in large posted data
Summary: XSS Auditor is spinning inside decodeURLEscapeSequences() if there are percen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 49845
Blocks:
  Show dependency tree / graph
 
Reported: 2011-01-30 15:15 PST by Alexey Proskuryakov
Modified: 2011-02-03 21:27 PST (History)
3 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.