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 LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dbates
Priority: P2 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
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.