Bug 17814 – Reading past end of string, for certain malformed <?xml ..?> tags

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Eric Roman]

This applies to the "Safari-3-1-branch".

when parsing malformed <?xml ...?> tags, strict bounds checking is not enforced, so can read past the end of string.

For example "http://www.exitfest.org" does not have a terminal question-mark:
 <?xml version="1.0" encoding="iso-8859-2">

And has caused a crash for me.

This problem looks to have been fixed in:
trunk/WebCore/loader/TextResourceDecoder.cpp @ r30535

Perhaps this is worth back-porting to the 3.1 branch.

[Eric Seidel (no email)]

Created attachment 19718 [details]
test case (crashes browser)

[Mark Rowe (bdash)]

<rdar://problem/5796571>

[Mark Rowe (bdash)]

This does not occur in trunk.

[Alexey Proskuryakov]

Where you running an instrumented build of some kind?

[Eric Roman]

The following test should repro the problem:

$ cat broken.html
<?xml version="1.0" encoding="iso-8859-2>

(Apologies, the example I posted earlier was for an outdated version of webkit)

I was running webkit with Purify tool on Windows, and it showed the problem to be in findXMLEncoding():

    // Find the trailing quotation mark.
    int end = pos;
    while (str[end] != quoteMark)    <----- Access Violation
        ++end;

Since the input contains no terminal quote mark, it reads past end of string.

In trunk I see that this is fixed, as the line now reads:

    while (end < len && str[end] != quoteMark)



(At some point I was repro-ing this with missing question mark too, but this seems to have gone away when I synched up my client recentish.)

[Mark Rowe (bdash)]

So in other words, the test case does *not* crash the browser but it *does* trigger errors from memory checking tools (Purify, Valgrind, etc)?  

[Oliver Hunt]

After keeping the test case as my homepage for a few days i eventually hit this crash:
0   com.apple.WebCore             	0x90989fa7 WebCore::TextResourceDecoder::checkForHeadCharset(char const*, unsigned long, bool&) + 935
1   com.apple.WebCore             	0x90989a47 WebCore::TextResourceDecoder::decode(char const*, unsigned long) + 679
2   com.apple.WebCore             	0x90987f7e WebCore::FrameLoader::write(char const*, int, bool) + 190
3   com.apple.WebCore             	0x909cab67 WebCore::FrameLoader::addData(char const*, int) + 39
4   com.apple.WebCore             	0x909c4ddd -[WebCoreFrameBridge receivedData:textEncodingName:] + 205
5   com.apple.WebKit              	0x9544c900 -[WebHTMLRepresentation receivedData:withDataSource:] + 224
6   com.apple.WebKit              	0x9544c7bb -[WebDataSource(WebInternal) _receivedData:] + 91
7   com.apple.WebKit              	0x9544c739 WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) + 137
8   com.apple.WebCore             	0x909c1d86 WebCore::DocumentLoader::commitLoad(char const*, int) + 70
9   com.apple.WebCore             	0x909c1925 WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) + 69
10  com.apple.WebCore             	0x909c1897 WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) + 71
11  com.apple.WebCore             	0x909c1848 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) + 56
12  com.apple.Foundation          	0x950383b7 -[NSURLConnection(NSURLConnectionReallyInternal) sendDidReceiveData:originalLength:] + 119
13  com.apple.Foundation          	0x9503831e _NSURLConnectionDidReceiveData + 94
14  com.apple.CFNetwork           	0x931a90af sendDidReceiveDataCallback + 518
15  com.apple.CFNetwork           	0x931a676d _CFURLConnectionSendCallbacks + 1559
16  com.apple.CFNetwork           	0x931a60d9 muxerSourcePerform + 283
17  com.apple.CoreFoundation      	0x933a862e CFRunLoopRunSpecific + 3166
18  com.apple.CoreFoundation      	0x933a8d18 CFRunLoopRunInMode + 88
19  com.apple.HIToolbox           	0x913906a0 RunCurrentEventLoopInMode + 283
20  com.apple.HIToolbox           	0x913904b9 ReceiveNextEventCommon + 374
21  com.apple.HIToolbox           	0x9139032d BlockUntilNextEventMatchingListInMode + 106
22  com.apple.AppKit              	0x942457d9 _DPSNextEvent + 657
23  com.apple.AppKit              	0x9424508e -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
24  com.apple.Safari              	0x0000804e 0x1000 + 28750
25  com.apple.AppKit              	0x9423e0c5 -[NSApplication run] + 795
26  com.apple.AppKit              	0x9420b30a NSApplicationMain + 574
27  com.apple.Safari              	0x000b9a76 0x1000 + 756342

Thread 1:
0   libSystem.B.dylib             	0x95e2cbce __semwait_signal + 10
1   libSystem.B.dylib             	0x95e578cd pthread_cond_wait$UNIX2003 + 73
2   com.apple.WebCore             	0x9094f56f WebCore::IconDatabase::syncThreadMainLoop() + 239
3   com.apple.WebCore             	0x90907cd5 WebCore::IconDatabase::iconDatabaseSyncThread() + 181
4   libSystem.B.dylib             	0x95e56c55 _pthread_start + 321
5   libSystem.B.dylib             	0x95e56b12 thread_start + 34

Thread 2:
0   libSystem.B.dylib             	0x95e259e6 mach_msg_trap + 10
1   libSystem.B.dylib             	0x95e2d1dc mach_msg + 72
2   com.apple.CoreFoundation      	0x933a80de CFRunLoopRunSpecific + 1806
3   com.apple.CoreFoundation      	0x933a8d18 CFRunLoopRunInMode + 88
4   com.apple.CFNetwork           	0x931a16cc CFURLCacheWorkerThread(void*) + 396
5   libSystem.B.dylib             	0x95e56c55 _pthread_start + 321
6   libSystem.B.dylib             	0x95e56b12 thread_start + 34

Thread 3:
0   libSystem.B.dylib             	0x95e259e6 mach_msg_trap + 10
1   libSystem.B.dylib             	0x95e2d1dc mach_msg + 72
2   com.apple.CoreFoundation      	0x933a80de CFRunLoopRunSpecific + 1806
3   com.apple.CoreFoundation      	0x933a8d18 CFRunLoopRunInMode + 88
4   com.apple.Foundation          	0x95036ac0 +[NSURLConnection(NSURLConnectionReallyInternal) _resourceLoadLoop:] + 320
5   com.apple.Foundation          	0x94fd35ad -[NSThread main] + 45
6   com.apple.Foundation          	0x94fd3154 __NSThread__main__ + 308
7   libSystem.B.dylib             	0x95e56c55 _pthread_start + 321
8   libSystem.B.dylib             	0x95e56b12 thread_start + 34

Thread 4:
0   libSystem.B.dylib             	0x95e75b3a select$DARWIN_EXTSN + 10
1   libSystem.B.dylib             	0x95e56c55 _pthread_start + 321
2   libSystem.B.dylib             	0x95e56b12 thread_start + 34

[Alexey Proskuryakov]

(In reply to comment #7)
> After keeping the test case as my homepage for a few days i eventually hit this
> crash:
> 0   com.apple.WebCore                   0x90989fa7
> WebCore::TextResourceDecoder::checkForHeadCharset(char const*, unsigned long,
> bool&) + 935

Are you sure this is the same issue?

[Alexey Proskuryakov]

There doesn't seem to be a pressing reason to backport this to 3.1 (for all we know, this is neither a security issue, nor a frequent crash). As this is known to be fixed in trunk, closing as FIXED. The test case is going to be landed as part of bug 21407.

As mentioned before, the issue Oliver is reporting was probably a different one, and we haven't seen it happen recently anyway.