Summary: | Reproducible crash in WebCore::SVGPreserveAspectRatio::parsePreserveAspectRatio in svg/W3C-SVG-1.1/animate-elem-40-t.svg under guard malloc | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | SVG | Assignee: | Darin Adler <darin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Major | CC: | davem, eric, mrowe, rwlbuis | ||||||
Priority: | P1 | Keywords: | LayoutTestFailure | ||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
URL: | http://build.webkit.org/post-commit-powerpc-mac-os-x/builds/5246 | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2007-01-22 09:20:42 PST
*** Bug 11408 has been marked as a duplicate of this bug. *** There are some cases in the function that clearly don't check the end of buffer. Mainly cases where skipOptionalSpaces is called, but its return value is ignored. However, I can't tell from reading the code which of the letters are allowed to be combined, so I'm not posting a patch. The four cases that are currently wrong are 'n' or 'x' after a 'd' 'm' or 's' after a 'd' or 'n' or 'x' Eric or Rob, please comment on Comment #3. These are the preserveAspectRatio parsing rules: http://www.w3.org/TR/SVG/coords.html#PreserveAspectRatioAttribute Ideally we would have a less error-prone way of parsing than the current copy/paste comparisons. I think this parser could just be simplified to use checkString calls at the top level ifs, or at least add a checkChar(currentPtr, end, deferDesc, 0) or similar. which checks the first char, as well as checks to make sure ptr < end. Something along those lines to make the parser easier to read/code w/o error. Also, to answer Darin's question, only the middle if section is required for the string to be valid. both "defer" and "meet-or-slice" are optional. Created attachment 12628 [details]
fix for buffer overrun
Someone should make test cases for Darin's untested fixes in this bug. Comment on attachment 12628 [details]
fix for buffer overrun
r- for missing tests to get this out of the review queue.
Created attachment 12661 [details]
patch to fix buffer overrun, including a new test and change log
Comment on attachment 12661 [details]
patch to fix buffer overrun, including a new test and change log
r=me
Mass removal of NeedsRadar keyword from my bugs that have already been RESOLVED. |