Bug 108698

Summary:

Continue making XSSAuditor thread safe: Remove dependency on parser's sourceForToken and TextResourceDecoder

Product:

WebKit

Reporter:

Tony Gentilcore <tonyg>

Component:

New Bugs

Assignee:

Tony Gentilcore <tonyg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dbates, ojan.autocc, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

106127, 107603

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Tony Gentilcore

Reported 2013-02-01 14:45:23 PST

Continue making XSSAuditor thread safe: Remove dependency on parser's sourceForToken and TextResourceDecoder

Attachments
Patch (22.67 KB, patch) 2013-02-01 14:46 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (23.22 KB, patch) 2013-02-01 14:55 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (25.21 KB, patch) 2013-02-02 16:31 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch (25.73 KB, patch) 2013-02-03 12:35 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch for landing (26.08 KB, patch) 2013-02-05 09:40 PST, Tony Gentilcore	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Tony Gentilcore

Comment 1 2013-02-01 14:46:51 PST

Created attachment 186143 [details] Patch

Tony Gentilcore

Comment 2 2013-02-01 14:55:44 PST

Created attachment 186151 [details] Patch

Adam Barth

Comment 3 2013-02-02 00:10:13 PST

Comment on attachment 186151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186151&action=review > Source/WebCore/html/parser/HTMLDocumentParser.h:-78 > - String sourceForToken(const HTMLToken&); Nice! > Source/WebCore/html/parser/XSSAuditor.cpp:275 > -PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token) > +PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token, HTMLSourceTracker& sourceTracker, const TextResourceDecoder* decoder) Can we put these things in a struct so that we don't need to pass all 3x the parameters to all these functions?

Tony Gentilcore

Comment 4 2013-02-02 16:31:32 PST

Created attachment 186240 [details] Patch

Tony Gentilcore

Comment 5 2013-02-02 16:33:37 PST

> > Source/WebCore/html/parser/XSSAuditor.cpp:275 > > -PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token) > > +PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token, HTMLSourceTracker& sourceTracker, const TextResourceDecoder* decoder) > > Can we put these things in a struct so that we don't need to pass all 3x the parameters to all these functions? Thank you, that struct is much cleaner! Especially since bug 108666 will add another member.

Sam Weinig

Comment 6 2013-02-02 16:58:34 PST

Comment on attachment 186240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186240&action=review > Source/WebCore/ChangeLog:35 > + * html/parser/HTMLDocumentParser.cpp: > + (WebCore::HTMLDocumentParser::pumpTokenizer): > + * html/parser/HTMLDocumentParser.h: > + * html/parser/XSSAuditor.cpp: > + (WebCore::XSSAuditor::filterToken): > + (WebCore::XSSAuditor::filterStartToken): > + (WebCore::XSSAuditor::filterCharacterToken): > + (WebCore::XSSAuditor::filterScriptToken): > + (WebCore::XSSAuditor::filterObjectToken): > + (WebCore::XSSAuditor::filterParamToken): > + (WebCore::XSSAuditor::filterEmbedToken): > + (WebCore::XSSAuditor::filterAppletToken): > + (WebCore::XSSAuditor::filterIframeToken): > + (WebCore::XSSAuditor::filterMetaToken): > + (WebCore::XSSAuditor::filterBaseToken): > + (WebCore::XSSAuditor::filterFormToken): > + (WebCore::XSSAuditor::eraseDangerousAttributesIfInjected): > + (WebCore::XSSAuditor::eraseAttributeIfInjected): > + (WebCore::XSSAuditor::decodedSnippetForName): > + (WebCore::XSSAuditor::decodedSnippetForAttribute): > + (WebCore::XSSAuditor::decodedSnippetForJavaScript): > + * html/parser/XSSAuditor.h: > + (WebCore): > + (WebCore::FilterTokenRequest::FilterTokenRequest): > + (FilterTokenRequest): > + (XSSAuditor): It would be nice to have some more detailed explanation here.

Build Bot

Comment 7 2013-02-02 17:35:40 PST

Comment on attachment 186240 [details] Patch Attachment 186240 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16356126

Build Bot

Comment 8 2013-02-02 18:39:56 PST

Comment on attachment 186240 [details] Patch Attachment 186240 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16353165

Tony Gentilcore

Comment 9 2013-02-03 12:35:35 PST

Created attachment 186266 [details] Patch

Tony Gentilcore

Comment 10 2013-02-03 12:35:58 PST

Comment on attachment 186240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186240&action=review >> Source/WebCore/ChangeLog:35 >> + (XSSAuditor): > > It would be nice to have some more detailed explanation here. Good idea. I beefed up the ChangeLog a bit.

Build Bot

Comment 11 2013-02-03 13:37:59 PST

Comment on attachment 186266 [details] Patch Attachment 186266 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16358673

Build Bot

Comment 12 2013-02-03 14:39:45 PST

Comment on attachment 186266 [details] Patch Attachment 186266 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16352772

Tony Gentilcore

Comment 13 2013-02-04 10:10:48 PST

Comment on attachment 186266 [details] Patch The win-ews failure looks unrelated to this patch.

Adam Barth

Comment 14 2013-02-04 21:29:16 PST

Comment on attachment 186266 [details] Patch Great! Thanks.

WebKit Review Bot

Comment 15 2013-02-04 21:33:33 PST

Comment on attachment 186266 [details] Patch Rejecting attachment 186266 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 186266, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: e/WebCore/html/parser/XSSAuditor.cpp Hunk #1 succeeded at 278 (offset 6 lines). Hunk #2 FAILED at 338. Hunk #3 succeeded at 573 (offset 9 lines). Hunk #4 succeeded at 630 (offset 9 lines). 1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/html/parser/XSSAuditor.cpp.rej patching file Source/WebCore/html/parser/XSSAuditor.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/16375537

Tony Gentilcore

Comment 16 2013-02-05 09:40:13 PST

Created attachment 186647 [details] Patch for landing

WebKit Review Bot

Comment 17 2013-02-05 09:57:26 PST

Comment on attachment 186647 [details] Patch for landing Clearing flags on attachment: 186647 Committed r141897: <http://trac.webkit.org/changeset/141897>

WebKit Review Bot

Comment 18 2013-02-05 09:57:30 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.