Bugzilla – Bug 15912
Case-sensitive handling of HTTP response headers causes network timeouts
Last modified: 2011-06-13 09:08:57 UTC
The code in SocketHttp.lua and RequestHttp.lua assumes case sensitive header field names, for example when checking for "Content-Length" or "Transfer-Encoding". RFC 2616 (HTTP/1.1) section 4.2 (http://tools.ietf.org/html/rfc2616#page-31) says: "Field names are case-insensitive." Some web servers (for example Sun's com.sun.net.httpserver.HttpServer) normalize the headers to "Content-length" and "Transfer-encoding" before sending, which results in SqueezePlay not parsing the content length and then timing out the body revive pump. Looking briefly at the code this problem could be fixed for example by normalizing the incoming header keys in RequestHttp's t_setResponseHeaders() so that they are always stored as "Content-Length" (or all lowercase), and then doing the same kind of key normalization in t_getResponseHeader(). Assuming these two functions are the only entry points to the response headers this should work, but there might be code using the header map directly (?). In that case normalizing to "Content-Length" would be preferable over lower-casing, as the existing code seems to use the form "Content-Length".
(In reply to comment #0) > Some web servers (for example Sun's com.sun.net.httpserver.HttpServer) > normalize the headers to "Content-length" and "Transfer-encoding" before > sending, which results in SqueezePlay not parsing the content length and then > timing out the body revive pump. That's _body receive pump_ ;)
Thanks for figuring out the problem, headers should indeed be handled in a case-insensitive way.
Created attachment 7229 [details] proposed patch Do not have time to test this now, so just parking it here.
Andy, can you take a quick look and confirm that you believe it is OK to check in?
I'd like to exercise Alan's proposed patch. Can anyone provide a method of doing that? I've added log() messages in the areas where the case transform occurs, and afaict I've not been able to trigger that code once. I've tried various music services, internet radio, flickr requests. Where is this section of code used?
apologies, forget the previous comment. I had to turn on net.http logging
added some log:warn()s to the code in Alan's patch: function t_getResponseHeader(self, key) if self.t_httpResponse.headers then log:warn('########################-----> ', key) log:warn('########################-----> ', string.lower(key)) log:warn('########################-----> ', self.t_httpResponse.headers[string.lower(key)]) return self.t_httpResponse.headers[string.lower(key)] else return nil end end console reports a lot of this, which looks about right 20110526 21:13:33.367 WARN net.http - RequestHttp.lua:301 ########################-----> Connection 20110526 21:13:33.367 WARN net.http - RequestHttp.lua:302 ########################-----> connection 20110526 21:13:33.367 WARN net.http - RequestHttp.lua:303 ########################-----> keep-alive but then I see others where the return value is nil, because it is not in the headers table: 20110526 21:14:01.988 WARN net.http - RequestHttp.lua:301 ########################-----> Transfer-Encoding 20110526 21:14:01.988 WARN net.http - RequestHttp.lua:302 ########################-----> transfer-encoding 20110526 21:14:01.988 WARN net.http - RequestHttp.lua:303 ########################-----> nil Everything appears to be working fine, but I have no idea what the implications of this is.... Alan, can you comment?
Tor, Alan, could either of you give me a pointer on how to reproduce the network timeout issue due to the case-sensitive HTTP handling? I've confirmed the patch does what it purports to do, which is to get around this issue by lowercasing the header keys. However, I'm not going to apply this patch if I can't have some evidence that it's fixing something that needs fixing. In the absence of a reproducible case for the bug, my recommendation is to not apply this patch.
(In reply to comment #8) > Tor, Alan, > > could either of you give me a pointer on how to reproduce the network timeout > issue due to the case-sensitive HTTP handling? You should be able to reproduce it by having SqueezePlay pull some data from a webserver that normalizes the headers, such as Sun's com.sun.net.httpserver.HttpServer. If I remember correctly the original issue I had was a plugin reporting the album cover-art as a url to a server with this behavior, and resulting in SqueezePlay now showing the cover art. Sadly I don't know the SqueezePlay code-base enough to provide you with a minimal testcase, sorry about that. > I've confirmed the patch does what it purports to do, which is to get around > this issue by lowercasing the header keys. However, I'm not going to apply this > patch if I can't have some evidence that it's fixing something that needs > fixing. > > In the absence of a reproducible case for the bug, my recommendation is to not > apply this patch. I don't really get that stance. The failing use-case I described above (missing/not showing cover-art), and the fact that the current behavior is not per RFC 2616's "Field names are case-insensitive." should be enough to convince you that it's in fact a bug and might lead to other problems down the road?
(In reply to comment #9) > If I remember correctly the original issue I > had was a plugin reporting the album cover-art as a url to a server with this > behavior, and resulting in SqueezePlay now showing the cover art. SqueezePlay _not_ showing the cover art.
== Auto-comment from SVN commit #9449 to the jive repo by bklaas == == http://svn.slimdevices.com/jive?view=revision&revision=9449 == Fixed Bug: 15912 Description: lowercase HTTP header strings
I checked the patch in. My reservations on this checkin are the following: 1. I never saw a single example of where the current code was causing an issue 2. calling string.lower() repeatedly in a lower level Lua file like RequestHttp.lua has to have _some_ performance cost. My guess is that the performance cost is very very low, but again, I'm fixing something I've never seen cause an issue. I fully understand the point about the RFC, but it sure would have felt better checking in a fix for something with a reproducible case. Regardless, it's checked in.