Reproducible crash in WebCore::SVGPreserveAspectRatio::parsePreserveAspectRatio in svg/W3C-SVG-1.1/animate-elem-40-t.svg under guard malloc

[David Kilzer (:ddkilzer)]

To reproduce:

./WebKitTools/Scripts/run-webkit-tests --debug --guard-malloc LayoutTests/svg/W3C-SVG-1.1/animate-elem-40-t.svg

Crashes with:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0xcb612000

Thread 0 Crashed:
0   com.apple.WebCore           0x010a6c49 WebCore::SVGPreserveAspectRatio::parsePreserveAspectRatio(WebCore::String con
st&) + 779 (SVGPreserveAspectRatio.cpp:151)
1   com.apple.WebCore           0x0108dacc WebCore::SVGImageElement::parseMappedAttribute(WebCore::MappedAttribute*) + 3
42 (SVGImageElement.cpp:74)
2   com.apple.WebCore           0x0123fc71 WebCore::StyledElement::attributeChanged(WebCore::Attribute*, bool) + 489 (St
yledElement.cpp:180)
3   com.apple.WebCore           0x010b06c0 WebCore::SVGStyledElement::attributeChanged(WebCore::Attribute*, bool) + 38 (
SVGStyledElement.cpp:226)
4   com.apple.WebCore           0x012450db WebCore::NamedAttrMap::addAttribute(WebCore::Attribute*) + 289 (NamedAttrMap.
cpp:289)
5   com.apple.WebCore           0x01248858 WebCore::Element::setAttribute(WebCore::QualifiedName const&, WebCore::String
Impl*, int&) + 368 (Element.cpp:397)
6   com.apple.WebCore           0x0124898a WebCore::Element::setAttributeNS(WebCore::String const&, WebCore::String cons
t&, WebCore::String const&, int&) + 202 (Element.cpp:807)
7   com.apple.WebCore           0x0102af5b WebCore::handleElementAttributes(WebCore::Element*, unsigned char const**, in
t, int&) + 431 (XMLTokenizer.cpp:625)
8   com.apple.WebCore           0x0102dff1 WebCore::XMLTokenizer::startElementNs(unsigned char const*, unsigned char con
st*, unsigned char const*, int, unsigned char const**, int, int, unsigned char const**) + 723 (XMLTokenizer.cpp:670)
9   com.apple.WebCore           0x0102e443 WebCore::startElementNsHandler(void*, unsigned char const*, unsigned char con
st*, unsigned char const*, int, unsigned char const**, int, int, unsigned char const**) + 95 (XMLTokenizer.cpp:985)
10  libxml2.2.dylib             0x9293dc9d xmlParseStartTag + 8465
11  libxml2.2.dylib             0x9291d69b xmlParseChunk + 1912
12  com.apple.WebCore           0x0102b15c WebCore::XMLTokenizer::write(WebCore::SegmentedString const&, bool) + 314 (XM
LTokenizer.cpp:570)
13  com.apple.WebCore           0x013b61d1 WebCore::FrameLoader::write(char const*, int, bool) + 923 (FrameLoader.cpp:88
2)
14  com.apple.WebCore           0x013b6303 WebCore::FrameLoader::addData(char const*, int) + 275 (FrameLoader.cpp:1498)
15  com.apple.WebCore           0x010fbe7b -[WebCoreFrameBridge addData:] + 163 (WebCoreFrameBridge.mm:295)
16  com.apple.WebCore           0x010ff43a -[WebCoreFrameBridge receivedData:textEncodingName:] + 250 (WebCoreFrameBridg
e.mm:1584)
17  com.apple.WebKit            0x00232441 -[WebHTMLRepresentation receivedData:withDataSource:] + 199 (WebHTMLRepresent
ation.mm:174)
18  com.apple.WebKit            0x0022dbdb -[WebDataSource(WebInternal) _receivedData:] + 89 (WebDataSource.mm:178)
19  com.apple.WebKit            0x00294495 WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, in
t) + 127 (WebFrameLoaderClient.mm:644)
20  com.apple.WebCore           0x013b2d7b WebCore::FrameLoader::committedLoad(WebCore::DocumentLoader*, char const*, in
t) + 53 (FrameLoader.cpp:2924)
21  com.apple.WebCore           0x013c2df9 WebCore::DocumentLoader::commitLoad(char const*, int) + 87 (DocumentLoader.cp
p:327)
22  com.apple.WebCore           0x013c2e52 WebCore::DocumentLoader::receivedData(char const*, int) + 76 (DocumentLoader.
cpp:340)
23  com.apple.WebCore           0x013b21f7 WebCore::FrameLoader::receivedData(char const*, int) + 41 (FrameLoader.cpp:18
89)
24  com.apple.WebCore           0x013c403c WebCore::MainResourceLoader::addData(char const*, int, bool) + 80 (MainResour
ceLoader.cpp:135)
25  com.apple.WebCore           0x013c5f01 WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) + 
83
26  com.apple.WebCore           0x013c4371 WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool
) + 281 (MainResourceLoader.cpp:304)
27  com.apple.WebCore           0x013c5b68 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*
, int, int) + 58
28  com.apple.WebCore           0x013a5aaa -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] 
+ 172 (ResourceHandleMac.mm:350)
29  com.apple.Foundation        0x9265eb86 -[NSURLConnection(NSURLConnectionInternal) _sendDidReceiveDataCallback] + 641
30  com.apple.Foundation        0x9265ce67 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 686
31  com.apple.Foundation        0x9265cb41 _sendCallbacks + 201
32  com.apple.CoreFoundation    0x9082afd2 CFRunLoopRunSpecific + 1213
33  com.apple.CoreFoundation    0x9082ab0e CFRunLoopRunInMode + 61
34  com.apple.Foundation        0x9262ddc6 -[NSRunLoop runMode:beforeDate:] + 182
35  DumpRenderTree              0x00008ed6 runTest + 943 (DumpRenderTree.m:1042)
36  DumpRenderTree              0x000060f9 dumpRenderTree + 3355 (DumpRenderTree.m:399)
37  DumpRenderTree              0x00006318 main + 70 (DumpRenderTree.m:450)
38  DumpRenderTree              0x00002102 _start + 216
39  DumpRenderTree              0x00002029 start + 41

See also the original description of bug 11108 and bug 11408.

[David Kilzer (:ddkilzer)]

*** Bug 11408 has been marked as a duplicate of this bug. ***

[Darin Adler]

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'

[David Kilzer (:ddkilzer)]

Eric or Rob, please comment on Comment #3.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Darin Adler]

Created attachment 12628 [details]
fix for buffer overrun

[Maciej Stachowiak]

Someone should make test cases for Darin's untested fixes in this bug.

[Maciej Stachowiak]

Comment on attachment 12628 [details]
fix for buffer overrun

r- for missing tests to get this out of the review queue.

[Darin Adler]

Created attachment 12661 [details]
patch to fix buffer overrun, including a new test and change log

[Maciej Stachowiak]

Comment on attachment 12661 [details]
patch to fix buffer overrun, including a new test and change log

r=me

[David Kilzer (:ddkilzer)]

Mass removal of NeedsRadar keyword from my bugs that have already been RESOLVED.