Bug 134661

Summary: CSS canvas color parsing accepts invalid color identifiers
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, commit-queue, darin, dbates, dino, esprehn+autocc, glenn, gyuyoung.kim, jfernandez, macpherson, menard, rego, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 134419    
Attachments:
Description Flags
Patch
none
Patch benjamin: review+

Javier Fernandez
Reported 2014-07-06 01:15:17 PDT
Current implementation of the CSSParser::parseSystemColor assumes that if a valid cssValueKeywordID is got from the color string then it has to be a valid color. Such assumption is wrong and lead to many bugs and layout test failures. On example of these failures is the one detected after trying the patch for the bug #134419, which adds a new ValueKey for the "true" string. This string is part of the invalid values testing case of the canvas-color-serialization.html Layout test.
Attachments
Patch (5.65 KB, patch)
2014-07-06 03:21 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (20.79 KB, patch)
2014-07-09 07:53 PDT, Javier Fernandez 		benjamin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Javier Fernandez
Comment 1 2014-07-06 03:21:00 PDT
Created attachment 234454 [details] Patch
Benjamin Poulain
Comment 2 2014-07-06 16:38:03 PDT
Comment on attachment 234454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234454&action=review Hilarious bug, that's a good catch. I disagree with your fix though. IMHO, we should never pass an invalid ID to systemColor(). You could check that the input ID is in the range alpha->-webkit-text. I would also like the same test for CSS style resolution in addition to canvas. I know CSSParser::parseSystemColor() is not used for CSS parsing, but it is better to have the test to be on the safe side if the code changes. Long term, someone should investigate if it would be better to split CSSValueKeywords into tiny perfect hash tables. That's completely out of scope here though :) > Source/WebCore/ChangeLog:9 > + that if a valid cssValueKeywordID is got from the color string "is got". > Source/WebCore/css/CSSParser.cpp:1362 > + color = parsedColor.rgb(); Changing the color when parsedColor.isValid() is false does not seem right.
Darin Adler
Comment 3 2014-07-06 17:07:40 PDT
(In reply to comment #2) > You could check that the input ID is in the range alpha->-webkit-text. That doesn’t sound right. I think it’s the job of the theme to decide which CSS identifiers are actually legal colors. What if one of the names of the colors was also an identifier that was used for another purpose elsewhere in CSS?
Benjamin Poulain
Comment 4 2014-07-06 17:15:07 PDT
(In reply to comment #3) > (In reply to comment #2) > > You could check that the input ID is in the range alpha->-webkit-text. > > That doesn’t sound right. I think it’s the job of the theme to decide which CSS identifiers are actually legal colors. What if one of the names of the colors was also an identifier that was used for another purpose elsewhere in CSS? I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()).
Benjamin Poulain
Comment 5 2014-07-06 17:16:37 PDT
> I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()). Well, obviously one can also add more branches in there :)
Darin Adler
Comment 6 2014-07-06 17:26:57 PDT
(In reply to comment #4) > I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()). Oh, maybe we should put that code somewhere we can share it!
Darin Adler
Comment 7 2014-07-06 17:28:50 PDT
Retitling since the bug is in a function only called by the canvas implementation. The title would be misleading if it claimed a broader problem with CSS color parsing.
Benjamin Poulain
Comment 8 2014-07-06 17:29:46 PDT
(In reply to comment #6) > (In reply to comment #4) > > I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()). > > Oh, maybe we should put that code somewhere we can share it! Yep, agreed. When I originally commented I thought that code was a simple check in range. But that's a little more complicated that that... It would be best to have this in a separate function.
Javier Fernandez
Comment 9 2014-07-09 07:53:31 PDT
Created attachment 234639 [details] Patch Applied suggested changes.
Benjamin Poulain
Comment 10 2014-07-09 11:28:23 PDT
Comment on attachment 234639 [details] Patch Awesome! Great tests!
Javier Fernandez
Comment 11 2014-07-09 14:56:54 PDT
Committed r170933: <http://trac.webkit.org/changeset/170933>
Note You need to log in before you can comment on or make changes to this bug.