RESOLVED FIXED 53405
XSS Auditor is spinning inside decodeURLEscapeSequences() if there are percent signs in large posted data 
https://bugs.webkit.org/show_bug.cgi?id=53405
Summary XSS Auditor is spinning inside decodeURLEscapeSequences() if there are percen...
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
DetailsFormatted DiffDiff
Patch (2.05 KB, patch)
2011-02-03 12:23 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
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.