mongrel unicorn - Jun 2010 - Read error: #<TypeError: can't modify frozen string> raised from HttpParser

Hey guys,

Started running unicorn in a production server like two weeks ago.
It''s been running smoothly, but looking at the logs found 44
exceptions like this:

E, [2010-06-02T16:17:15.117071 #22680] ERROR -- : Read error:
#<TypeError: can''t modify frozen string>
E, [2010-06-02T16:17:15.117270 #22680] ERROR -- :
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
`headers''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
`read''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:643:in
`process_client''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:716:in
`worker_loop''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:714:in `each''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:714:in
`worker_loop''
/u/apps/xxxx/releases/20100602004926/vendor/plugins/newrelic_rpm/lib/new_relic/agent/instrumentation/unicorn_instrumentation.rb:7:in
`call''
/u/apps/xxxx/releases/20100602004926/vendor/plugins/newrelic_rpm/lib/new_relic/agent/instrumentation/unicorn_instrumentation.rb:7:in
`worker_loop''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:605:in
`spawn_missing_workers''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:602:in `fork''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:602:in
`spawn_missing_workers''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:598:in `each''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:598:in
`spawn_missing_workers''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:612:in
`maintain_worker_count''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:276:in
`start''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn.rb:28:in `run''
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/bin/unicorn_rails:203
/usr/bin/unicorn_rails:26:in `load''
/usr/bin/unicorn_rails:26

Ruby version: ruby 1.8.7 (2009-12-24 patchlevel 248) [i686-linux],
MBARI 0x8770, Ruby Enterprise Edition 2010.01


Any ideas? Maybe a bug in the http parser?

Thanks,
Augusto

Augusto Becciu <augusto at jadedpixel.com> wrote:
> Hey guys,
> 
> Started running unicorn in a production server like two weeks ago.
> It''s been running smoothly, but looking at the logs found 44
> exceptions like this:
> 
> E, [2010-06-02T16:17:15.117071 #22680] ERROR -- : Read error:
> #<TypeError: can''t modify frozen string>
> E, [2010-06-02T16:17:15.117270 #22680] ERROR -- :
>
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
> `headers''
<snip>
> Ruby version: ruby 1.8.7 (2009-12-24 patchlevel 248) [i686-linux],
> MBARI 0x8770, Ruby Enterprise Edition 2010.01
> 
> Any ideas? Maybe a bug in the http parser?
Hi Augusto,

Somehow the reusable Unicorn::HttpRequest::BUF string constant is
getting frozen when it shouldn''t be.   Do you have any code that could
be freezing that string?  That string object should never be returned to
the application via any code paths in Unicorn (env or tee_input).

-- 
Eric Wong

Hi Eric,

There''s no way our application could be freezing that constant, at
least not intentionally.

We''re using New Replic''s RPM plugin, but checked it out and
couldn''t
find anything that could do that.

http://github.com/newrelic/rpm/blob/master/lib/new_relic/agent/instrumentation/unicorn_instrumentation.rb

Let me know if I can help in any way.

Thanks,
Augusto

On Wed, Jun 2, 2010 at 5:25 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Augusto Becciu <augusto at jadedpixel.com> wrote:
>> Hey guys,
>>
>> Started running unicorn in a production server like two weeks ago.
>> It''s been running smoothly, but looking at the logs found 44
>> exceptions like this:
>>
>> E, [2010-06-02T16:17:15.117071 #22680] ERROR -- : Read error:
>> #<TypeError: can''t modify frozen string>
>> E, [2010-06-02T16:17:15.117270 #22680] ERROR -- :
>>
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
>> `headers''
>
> <snip>
>
>> Ruby version: ruby 1.8.7 (2009-12-24 patchlevel 248) [i686-linux],
>> MBARI 0x8770, Ruby Enterprise Edition 2010.01
>>
>> Any ideas? Maybe a bug in the http parser?
>
> Hi Augusto,
>
> Somehow the reusable Unicorn::HttpRequest::BUF string constant is
> getting frozen when it shouldn''t be. ? Do you have any code that
could
> be freezing that string? ?That string object should never be returned to
> the application via any code paths in Unicorn (env or tee_input).
>
> --
> Eric Wong
>

Augusto Becciu <augusto at jadedpixel.com> wrote:
> On Wed, Jun 2, 2010 at 5:25 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> > Augusto Becciu <augusto at jadedpixel.com> wrote:
> >> Hey guys,
> >>
> >> Started running unicorn in a production server like two weeks ago.
> >> It''s been running smoothly, but looking at the logs found
44
> >> exceptions like this:
> >>
> >> E, [2010-06-02T16:17:15.117071 #22680] ERROR -- : Read error:
> >> #<TypeError: can''t modify frozen string>
> >> E, [2010-06-02T16:17:15.117270 #22680] ERROR -- :
> >>
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
> >> `headers''
> >
> > <snip>
> >
> >> Ruby version: ruby 1.8.7 (2009-12-24 patchlevel 248) [i686-linux],
> >> MBARI 0x8770, Ruby Enterprise Edition 2010.01
> >>
> >> Any ideas? Maybe a bug in the http parser?
> >
> > Hi Augusto,
> >
> > Somehow the reusable Unicorn::HttpRequest::BUF string constant is
> > getting frozen when it shouldn''t be. ? Do you have any code
that could
> > be freezing that string? ?That string object should never be returned
to
> > the application via any code paths in Unicorn (env or tee_input).
Please don''t top post, thanks.
> Hi Eric,
> 
> There''s no way our application could be freezing that constant, at
> least not intentionally.
> 
> We''re using New Replic''s RPM plugin, but checked it out
and couldn''t
> find anything that could do that.
> 
>
http://github.com/newrelic/rpm/blob/master/lib/new_relic/agent/instrumentation/unicorn_instrumentation.rb
> 
> Let me know if I can help in any way.
Are you able to reproduce the problem without the RPM plugin?  I''ve
never used RPM myself, but we''ve heard of (and proposed some fixes)
with it over the recent months.

Some of those problems could be segfaults (on x86_64), but memory
corruption could also cause an unintentional freeze, as well...

In particular, could you try disabling compression when sending things
upstream?

(totally untested, I don''t even have a New Relic account[1]).

diff --git a/lib/new_relic/agent/agent.rb b/lib/new_relic/agent/agent.rb
index 928c6d7..5e60520 100644
--- a/lib/new_relic/agent/agent.rb
+++ b/lib/new_relic/agent/agent.rb
@@ -554,7 +554,7 @@ module NewRelic
       dump_size = dump.size
       
       # small payloads don''t need compression      
-      return [dump, ''identity''] if dump_size < 2000
+      return [dump, ''identity'']
       
       # medium payloads get fast compression, to save CPU
       # big payloads get all the compression possible, to stay under
---

There''s also 1787b22eb2d8ab8b4046ae14be349aa487abc7b5 in the
v2.12.2_beta tag of git://github.com/newrelic/rpm which raises
the compression threshold, too...

-- 
Eric Wong

[1] - yes I''m allergic to signing up for commercial things

On Wed, Jun 2, 2010 at 6:38 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Augusto Becciu <augusto at jadedpixel.com> wrote:
>> On Wed, Jun 2, 2010 at 5:25 PM, Eric Wong <normalperson at
yhbt.net> wrote:
>> > Augusto Becciu <augusto at jadedpixel.com> wrote:
>> >> Hey guys,
>> >>
>> >> Started running unicorn in a production server like two weeks
ago.
>> >> It''s been running smoothly, but looking at the logs
found 44
>> >> exceptions like this:
>> >>
>> >> E, [2010-06-02T16:17:15.117071 #22680] ERROR -- : Read error:
>> >> #<TypeError: can''t modify frozen string>
>> >> E, [2010-06-02T16:17:15.117270 #22680] ERROR -- :
>> >>
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
>> >> `headers''
>> >
>> > <snip>
>> >
>> >> Ruby version: ruby 1.8.7 (2009-12-24 patchlevel 248)
[i686-linux],
>> >> MBARI 0x8770, Ruby Enterprise Edition 2010.01
>> >>
>> >> Any ideas? Maybe a bug in the http parser?
>> >
>> > Hi Augusto,
>> >
>> > Somehow the reusable Unicorn::HttpRequest::BUF string constant is
>> > getting frozen when it shouldn''t be. ? Do you have any
code that could
>> > be freezing that string? ?That string object should never be
returned to
>> > the application via any code paths in Unicorn (env or tee_input).
>
> Please don''t top post, thanks.
>
>> Hi Eric,
>>
>> There''s no way our application could be freezing that
constant, at
>> least not intentionally.
>>
>> We''re using New Replic''s RPM plugin, but checked it
out and couldn''t
>> find anything that could do that.
>>
>>
http://github.com/newrelic/rpm/blob/master/lib/new_relic/agent/instrumentation/unicorn_instrumentation.rb
>>
>> Let me know if I can help in any way.
>
> Are you able to reproduce the problem without the RPM plugin?
?I''ve
> never used RPM myself, but we''ve heard of (and proposed some
fixes)
> with it over the recent months.
>
> Some of those problems could be segfaults (on x86_64), but memory
> corruption could also cause an unintentional freeze, as well...
>
> In particular, could you try disabling compression when sending things
> upstream?
>
> (totally untested, I don''t even have a New Relic account[1]).
>
> diff --git a/lib/new_relic/agent/agent.rb b/lib/new_relic/agent/agent.rb
> index 928c6d7..5e60520 100644
> --- a/lib/new_relic/agent/agent.rb
> +++ b/lib/new_relic/agent/agent.rb
> @@ -554,7 +554,7 @@ module NewRelic
> ? ? ? dump_size = dump.size
>
> ? ? ? # small payloads don''t need compression
> - ? ? ?return [dump, ''identity''] if dump_size < 2000
> + ? ? ?return [dump, ''identity'']
>
> ? ? ? # medium payloads get fast compression, to save CPU
> ? ? ? # big payloads get all the compression possible, to stay under
> ---
>
> There''s also 1787b22eb2d8ab8b4046ae14be349aa487abc7b5 in the
> v2.12.2_beta tag of git://github.com/newrelic/rpm which raises
> the compression threshold, too...
>
> --
> Eric Wong
>
> [1] - yes I''m allergic to signing up for commercial things
>
Thanks Eric! Unfortunately after completely disabling RPM, we keep
getting that error. :(

What else could it be?

Augusto Becciu <augusto at jadedpixel.com> wrote:
> On Wed, Jun 2, 2010 at 6:38 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> > Augusto Becciu <augusto at jadedpixel.com> wrote:
> >> On Wed, Jun 2, 2010 at 5:25 PM, Eric Wong <normalperson at
yhbt.net> wrote:
> >> > Augusto Becciu <augusto at jadedpixel.com> wrote:
> >> >> Hey guys,
> >> >>
> >> >> Started running unicorn in a production server like two
weeks ago.
> >> >> It''s been running smoothly, but looking at the
logs found 44
> >> >> exceptions like this:
> >> >>
> >> >> E, [2010-06-02T16:17:15.117071 #22680] ERROR -- : Read
error:
> >> >> #<TypeError: can''t modify frozen string>
> >> >> E, [2010-06-02T16:17:15.117270 #22680] ERROR -- :
> >> >>
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
> >> >> `headers''
> >> >
> >> > <snip>
> >> >
> >> >> Ruby version: ruby 1.8.7 (2009-12-24 patchlevel 248)
[i686-linux],
> >> >> MBARI 0x8770, Ruby Enterprise Edition 2010.01
<snip>
> >> There''s no way our application could be freezing that
constant, at
> >> least not intentionally.
<snip>
> Thanks Eric! Unfortunately after completely disabling RPM, we keep
> getting that error. :(
> 
> What else could it be?
Any details about your application you''re allowed to share can help us,
especially a list of library/gem dependencies and versions.

I would grep through your gems and vendor directories to ensure there''s
nothing freezing objects it shouldn''t freeze.  It could also be a buggy
C extension corrupting memory somewhere, too...

This isn''t happening for all your worker processes, is it?

If your application logs show which PID handled each request, you can
join the PIDs from the application logs with PIDs in the error logs
generated by Unicorn.  That lets you narrow down the possible requests
that could trigger the errors and help you reproduce the problem sooner.

This is one of my favorite features of one-request-per-process model
is that you can easily narrow down which request is triggering a
particular bug without worrying about race conditions from other
requests.

This is certainly the first time I''ve heard of this problem, so I think
it''s specific to something in your application/environment.

-- 
Eric Wong

On Thu, Jun 3, 2010 at 8:40 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Augusto Becciu <augusto at jadedpixel.com> wrote:
>> On Wed, Jun 2, 2010 at 6:38 PM, Eric Wong <normalperson at
yhbt.net> wrote:
>> > Augusto Becciu <augusto at jadedpixel.com> wrote:
>> >> On Wed, Jun 2, 2010 at 5:25 PM, Eric Wong <normalperson at
yhbt.net> wrote:
>> >> > Augusto Becciu <augusto at jadedpixel.com> wrote:
>> >> >> Hey guys,
>> >> >>
>> >> >> Started running unicorn in a production server like
two weeks ago.
>> >> >> It''s been running smoothly, but looking at
the logs found 44
>> >> >> exceptions like this:
>> >> >>
>> >> >> E, [2010-06-02T16:17:15.117071 #22680] ERROR -- :
Read error:
>> >> >> #<TypeError: can''t modify frozen
string>
>> >> >> E, [2010-06-02T16:17:15.117270 #22680] ERROR -- :
>> >> >>
/usr/lib/ruby/gems/1.8/gems/unicorn-0.99.0/lib/unicorn/http_request.rb:59:in
>> >> >> `headers''
>> >> >
>> >> > <snip>
>> >> >
>> >> >> Ruby version: ruby 1.8.7 (2009-12-24 patchlevel 248)
[i686-linux],
>> >> >> MBARI 0x8770, Ruby Enterprise Edition 2010.01
>
> <snip>
>
>> >> There''s no way our application could be freezing that
constant, at
>> >> least not intentionally.
>
> <snip>
>
>> Thanks Eric! Unfortunately after completely disabling RPM, we keep
>> getting that error. :(
>>
>> What else could it be?
>
> Any details about your application you''re allowed to share can
help us,
> especially a list of library/gem dependencies and versions.
>
> I would grep through your gems and vendor directories to ensure
there''s
> nothing freezing objects it shouldn''t freeze. ?It could also be a
buggy
> C extension corrupting memory somewhere, too...
>
> This isn''t happening for all your worker processes, is it?
>
> If your application logs show which PID handled each request, you can
> join the PIDs from the application logs with PIDs in the error logs
> generated by Unicorn. ?That lets you narrow down the possible requests
> that could trigger the errors and help you reproduce the problem sooner.
>
> This is one of my favorite features of one-request-per-process model
> is that you can easily narrow down which request is triggering a
> particular bug without worrying about race conditions from other
> requests.
>
> This is certainly the first time I''ve heard of this problem, so I
think
> it''s specific to something in your application/environment.
>
> --
> Eric Wong
>
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.

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.

Below is a proposed patch.

Thanks a lot for your help!
Augusto


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

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 at
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

That looks perfect. Thanks Eric!

On Mon, Jun 7, 2010 at 11:45 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> 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 at
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
>