Read error: #<TypeError: can't modify frozen string> raised from HttpParser

Eric Wong normalperson at yhbt.net
Mon Jun 7 22:45:28 EDT 2010


Augusto Becciu <augusto at jadedpixel.com> wrote:
> Hi Eric,
> 
> Thanks for the ideas.
> 
> Ended up adding some debugging code to unicorn that lead me to the
> culprit of the problem. It turned out being a bug in the HttpParser.

Thank you Augusto,

Your explanation was good so I'll just use it as a commit message
and credit you as the author.  See the patch below and let me know
if everything looks alright before I push it out.

I have a few small things to go over, but I'll be releasing 0.999.0
tonight (which I hope will be the last before we finally celebrate
have a 1.0.0 release).

>From 7d2295fa774d1c98dfbde2b09d93d58712253d24 Mon Sep 17 00:00:00 2001
From: Augusto Becciu <augusto at jadedpixel.com>
Date: Mon, 7 Jun 2010 23:02:43 -0300
Subject: [PATCH] http: ignore Version: header if explicitly set by client

When Unicorn receives a request with a "Version" header, the
HttpParser transforms it into "HTTP_VERSION". After that tries to add
it to the request hash which already contains a "HTTP_VERSION" key
with the actual http version of the request. So it tries to append the
new value separated by a comma. But since the http version is a
freezed constant, the TypeError exception is raised.

According to the HTTP RFC
(http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.1) a
"Version" header is valid. However, it's not supported in rack, since
rack has a HTTP_VERSION env variable for the http version. So I think
the easiest way to deal with this problem is to just ignore the header
since it is extremely unusual. We were getting it from a crappy bot.

ref: http://mid.gmane.org/AANLkTimuGgcwNAMcVZdViFWdF-UcW_RGyZAue7phUXps@mail.gmail.com

Acked-by: Eric Wong <normalperson at yhbt.net>
---
 ext/unicorn_http/unicorn_http.rl |    9 +++++++++
 test/unit/test_http_parser_ng.rb |   20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
index 264db68..aa23024 100644
--- a/ext/unicorn_http/unicorn_http.rl
+++ b/ext/unicorn_http/unicorn_http.rl
@@ -197,6 +197,15 @@ static void write_value(VALUE req, struct http_parser *hp,
     assert_frozen(f);
   }
 
+  /*
+   * ignore "Version" headers since they conflict with the HTTP_VERSION
+   * rack env variable.
+   */
+  if (rb_str_cmp(f, g_http_version) == 0) {
+    hp->cont = Qnil;
+    return;
+  }
+
   e = rb_hash_aref(req, f);
   if (NIL_P(e)) {
     hp->cont = rb_hash_aset(req, f, v);
diff --git a/test/unit/test_http_parser_ng.rb b/test/unit/test_http_parser_ng.rb
index a067ac8..cb30f32 100644
--- a/test/unit/test_http_parser_ng.rb
+++ b/test/unit/test_http_parser_ng.rb
@@ -440,4 +440,24 @@ class HttpParserNgTest < Test::Unit::TestCase
     end
   end
 
+  def test_ignore_version_header
+    http = "GET / HTTP/1.1\r\nVersion: hello\r\n\r\n"
+    req = {}
+    assert_equal req, @parser.headers(req, http)
+    assert_equal '', http
+    expect = {
+      "SERVER_NAME" => "localhost",
+      "rack.url_scheme" => "http",
+      "REQUEST_PATH" => "/",
+      "SERVER_PROTOCOL" => "HTTP/1.1",
+      "PATH_INFO" => "/",
+      "HTTP_VERSION" => "HTTP/1.1",
+      "REQUEST_URI" => "/",
+      "SERVER_PORT" => "80",
+      "REQUEST_METHOD" => "GET",
+      "QUERY_STRING" => ""
+    }
+    assert_equal expect, req
+  end
+
 end
-- 
Eric Wong


More information about the mongrel-unicorn mailing list