Bug 56345 – REGRESSION: Crash in adjustMIMETypeIfNecessary since r81001

• Component: New Bugs
• Status: RESOLVED FIXED

[Jon]

Crash while navigating to the linked site results in the following stack trace.


Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.CoreFoundation      	0x00007fff817bbeb0 CFStringGetLength + 80
1   com.apple.CoreFoundation      	0x00007fff817d0b2b CFStringHasPrefix + 27
2   com.apple.WebCore             	0x0000000101637183 WebCore::adjustMIMETypeIfNecessary(_CFURLResponse*) + 595 (RetainPtr.h:63)
3   com.apple.WebCore             	0x0000000101505b18 -[WebCoreResourceHandleAsDelegate connection:didReceiveResponse:] + 136 (ResourceHandleMac.mm:891)
4   com.apple.Foundation          	0x00007fff840e8b67 _NSURLConnectionDidReceiveResponse + 123
5   com.apple.CFNetwork           	0x00007fff80f3aba4 URLConnectionClient::_clientSendDidReceiveResponse(_CFURLResponse*, URLConnectionClient::ClientConnectionEventQueue*) + 38
6   com.apple.CFNetwork           	0x00007fff80fa1a78 URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) + 456
7   com.apple.CFNetwork           	0x00007fff80fa1c1a URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) + 874
8   com.apple.CFNetwork           	0x00007fff80f28825 URLConnectionClient::processEvents() + 121
9   com.apple.CFNetwork           	0x00007fff80f28600 MultiplexerSource::perform() + 160
10  com.apple.CoreFoundation      	0x00007fff81802401 __CFRunLoopDoSources0 + 1361
11  com.apple.CoreFoundation      	0x00007fff818005f9 __CFRunLoopRun + 873
12  com.apple.CoreFoundation      	0x00007fff817ffdbf CFRunLoopRunSpecific + 575
13  com.apple.HIToolbox           	0x00007fff879d77ee RunCurrentEventLoopInMode + 333
14  com.apple.HIToolbox           	0x00007fff879d75f3 ReceiveNextEventCommon + 310
15  com.apple.HIToolbox           	0x00007fff879d74ac BlockUntilNextEventMatchingListInMode + 59
16  com.apple.AppKit              	0x00007fff86527e64 _DPSNextEvent + 718
17  com.apple.AppKit              	0x00007fff865277a9 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 155
18  com.apple.Safari              	0x000000010001605a 0x100000000 + 90202
19  com.apple.AppKit              	0x00007fff864ed48b -[NSApplication run] + 395
20  com.apple.AppKit              	0x00007fff864e61a8 NSApplicationMain + 364
21  com.apple.Safari              	0x0000000100009f7c 0x100000000 + 40828

[Pratik Solanki]

Caused by my change in r80975 made for bug 55912.

[Pratik Solanki]

The code tries to check if the "Content-Type" header in the response is "text/plain". But we don't do a NULL check here which means if the response did not have Content-Type set, we crash. Indeed, on that web page I am seeing a resource with the following response headers

Cache-Control:private, no-cache, no-cache=Set-Cookie, no-store, proxy-revalidate
Connection:keep-alive
Content-Length:0
Date:Tue, 15 Mar 2011 00:21:23 GMT
Expires:Mon, 01 Jan 1990 00:00:00 GMT
P3p:policyref="/w3c/p3p.xml", CP="NOI DSP COR NID OUR IND COM STA OTC"
Pragma:no-cache
Server:CS
Set-Cookie:UID=14d8c28-205.180.175.167-1249085941; expires=Thu, 14-Mar-2013 00:21:23 GMT; path=/; domain=.scorecardresearch.com

In the debugger I see

525	            if (CFStringHasPrefix(contentType.get(), CFSTR("text/plain"))) {
(gdb) p contentType
$1 = {
  m_ptr = 0x0
}

No Content-Type header present. This was fine in Obj-C when messaging nil "just worked", but in CF code we need to do the NULL check.

[Brady Eidson]

Actually, you'll need to double check this manually.

Just because the server doesn't respond with content-type, we still expect content type in most cases;  CFNetwork should always do content type sniffing for remote content.

Please double check and make sure the NSURLResponse you get *also* doesn't have a content type - we've definitely had cases in the past where the NSURL* and CFURL* code paths give different answers.

[Pratik Solanki]

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x00007fff85603eb0 in CFStringGetLength ()
Yes, the NSURLResponse doesn't have a Content-Type as well.

(gdb) up
#1  0x00007fff85618b2b in CFStringHasPrefix ()
(gdb) up
#2  0x00000001023506ab in WebCore::adjustMIMETypeIfNecessary (cfResponse=0x11f0ed6b0) at /Volumes/Data/psolanki/sources/external/WebKit/Source/WebCore/platform/network/mac/WebCoreURLResponse.mm:525
525	            if (CFStringHasPrefix(contentType.get(), CFSTR("text/plain"))) {
(gdb) p contentType
$1 = {
  m_ptr = 0x0
}
(gdb) po cfResponse 
<CFURLResponse 0x11f0ed6b0 [0x7fff70c26ee0]>{url = http://cim.meebo.com/cmd/tc}
(gdb) up
#3  0x0000000102170a66 in -[WebCoreResourceHandleAsDelegate connection:didReceiveResponse:] (self=0x121bd2000, _cmd=0x7fff810dbfff, connection=0x121bd41f0, r=0x1210f0a50) at /Volumes/Data/psolanki/sources/external/WebKit/Source/WebCore/platform/network/mac/ResourceHandleMac.mm:889
889	        adjustMIMETypeIfNecessary([r _CFURLResponse]);
(gdb) p r
$2 = (NSURLResponse *) 0x1210f0a50
(gdb) po [r allHeaderFields]
{
    Connection = "keep-alive";
    Date = "Tue, 15 Mar 2011 01:44:02 GMT";
    Server = "nginx/0.7.62";
    "Transfer-Encoding" = Identity;
}
(gdb)

[Pratik Solanki]

Created attachment 85757 [details]
Patch

[Adam Barth]

Comment on attachment 85757 [details]
Patch

Can we add a test for this change?  If we can't add a test, please explain why in the ChangeLog.  Thanks!

[Pratik Solanki]

<rdar://problem/9139469>

[Pratik Solanki]

I tried creating a testcase using php as well as .asis files. I just can't seem to trigger the scenario in a layout test. If I am reading the code correctly, I need a case where CFNetwork gives us a nil (or application/octet-stream) mime type AND the HTTP header does not have a Content-Type field.

[Alexey Proskuryakov]

What happens if you store the actual response that triggers this on insidetv.ew.com into an asis file?

[Pratik Solanki]

It's an XHR Request. CFNetwork gives us a nil MIME type on the response. And the headers don't have Content-Type.

Request URL:http://cim.meebo.com/cmd/tc
Request Method:POST
Status Code:200 OK

Request Headers:
Content-Type:application/x-www-form-urlencoded; charset=UTF-8
If-Modified-Since:Wed Dec 31 1969 16:00:00 GMT-0800 (PST)
Origin:http://cim.meebo.com
Referer:http://cim.meebo.com/cim/sandbox.php?lang=en&version=v89_cim_10_3_7&protocol=http%3A&network=ew
User-Agent:Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-us) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4

Form Data:
bcookie:3a0939be3a7dbfce1983_nd
tcookie:255f218fe00f262af82f
partner:ew

Response Headers:
Connection:keep-alive
Date:Wed, 16 Mar 2011 05:10:48 GMT
Server:nginx/0.7.62
Transfer-Encoding:Identity

[Pratik Solanki]

(In reply to comment #9)
> What happens if you store the actual response that triggers this on insidetv.ew.com into an asis file?

I tried. And it didn't seem to work. I keep getting text/html. Maybe I'm doing something wrong?

[Alexey Proskuryakov]

There are two things that could go wrong. First, the response may not match what we are getting from the real server, and then, interpreting it may hit a different code path. The former is easier to check - you can try requesting the resource from a local server with curl:

run-webkit-httpd
curl -i http://127.0.0.1:8000/path/resource.asis

For the latter - can you make a reduced test that hits this assertion after making a request to an actual remote server? It won't work from DumpRenderTree, because it blocks remote requests, but should work from Safari.

Note that Content-Type sniffing is supposed to be disabled for XMLHttpRequest responses. If you're getting a Content-Type that's not in response headers, that's worth a very deep look!

[Pratik Solanki]

(In reply to comment #12)
> There are two things that could go wrong. First, the response may not match what we are getting from the real server, and then, interpreting it may hit a different code path. The former is easier to check - you can try requesting the resource from a local server with curl:
> 
> run-webkit-httpd
> curl -i http://127.0.0.1:8000/path/resource.asis

Here's my .asis file


pratiksolanki@bifzi $ cat resources/noContentType.asis Cache-Control:private
Connection:keep-alive
Content-Length:100
Date:Wed, 16 Mar 2011 01:45:52 GMT
X-Appserver:app95
X-Aspnet-Version:2.0.50727
X-Cache:MISS from cap92_2
X-Cache-Lookup:MISS from cap92_2:2002
X-Powered-By:ASP.NET

????DG?G???(!<arch>
.... binary dump ....

Here's what curl says

$ curl -i http://127.0.0.1:8000/xmlhttprequest/resources/noContentType.asis
HTTP/1.1 200 OK
Date: Wed, 16 Mar 2011 07:44:12 GMT
Server: Apache/2.2.15 (Unix) PHP/5.3.3 mod_ssl/2.2.15 OpenSSL/0.9.8l
Cache-Control: private
Connection: keep-alive, close
X-Appserver: app95
X-Aspnet-Version: 2.0.50727
X-Cache: MISS from cap92_2
X-Cache-Lookup: MISS from cap92_2:2002
X-Powered-By: ASP.NET
Content-Length: 14760
Content-Type: text/plain

????DG?G???(!<arch>
....

So how did text/plain get added? And how can I remove that?

[Alexey Proskuryakov]

Yeah, it gets added automatically. Curiously, a web search for 'apache "no content-type"' returns both complaints from people who get no Content-Type, as well as unanswered questions about how to achieve that intentionally!

[Brady Eidson]

Just a bizarre idea - Does your test case have a file extension?  If so, remove it.
My reading indicates Apache will be less likely to try and throw a mime-type on there if it can't look up the extension.

[Pratik Solanki]

(In reply to comment #15)
> Just a bizarre idea - Does your test case have a file extension?  If so, remove it.
> My reading indicates Apache will be less likely to try and throw a mime-type on there if it can't look up the extension.

I need the extension to be .asis so that apache treats it as such and doesn't add its own headers. If I remove the extension then I get

$ curl -i http://127.0.0.1:8000/xmlhttprequest/resources/noContentType
HTTP/1.1 200 OK
Date: Wed, 16 Mar 2011 16:36:59 GMT
Server: Apache/2.2.15 (Unix) PHP/5.3.3 mod_ssl/2.2.15 OpenSSL/0.9.8l
Last-Modified: Wed, 16 Mar 2011 16:27:13 GMT
ETag: "57d86b-3a8d-49e9c046a3a40"
Accept-Ranges: bytes
Content-Length: 14989
Connection: close
Content-Type: text/plain

Cache-Control:private
Connection:keep-alive
Content-Length:100
Date:Wed, 16 Mar 2011 01:45:52 GMT
X-Appserver:app95
X-Aspnet-Version:2.0.50727
X-Cache:MISS from cap92_2
X-Cache-Lookup:MISS from cap92_2:2002
X-Powered-By:ASP.NET

????DG?G???(!<arch>
....

[Alexey Proskuryakov]

You can always force a handler with AddHandler directive in a .htaccess file regardless of extension, but I doubt that leaving out an extension will help anyway.

[Pratik Solanki]

Ok. I can get apache to not set Content-Type by adding the following to .htaccess

<Files "noContentType.asis">
DefaultType None
</Files>

Patch coming up.

[Pratik Solanki]

Created attachment 85947 [details]
Patch

[Pratik Solanki]

Committed r81267: <http://trac.webkit.org/changeset/81267>