undefined method `include?' for nil:NilClass (NoMethodError)

undefined method `include?' for nil:NilClass (NoMethodError)

[Owen Ou]

Hi,


We recently upgraded to Unicorn 5.0 but getting the following error:


[2015-11-16T14:54:16.943652 #19838] ERROR -- : app error: undefined
method `include?' for nil:NilClass (NoMethodError)

E, [2015-11-16T14:54:16.943712 #19838] ERROR -- :
/home/api/vendor/bundle/ruby/2.2.0/gems/unicorn-5.0.0/lib/unicorn/http_response.rb:40:in
`block in http_response_write'

E, [2015-11-16T14:54:16.943737 #19838] ERROR -- :
/home/api/vendor/bundle/ruby/2.2.0/gems/rack-1.6.4/lib/rack/utils.rb:490:in
`block in each'

E, [2015-11-16T14:54:16.943753 #19838] ERROR -- :
/home/api/vendor/bundle/ruby/2.2.0/gems/rack-1.6.4/lib/rack/utils.rb:489:in
`each'

E, [2015-11-16T14:54:16.943767 #19838] ERROR -- :
/home/api/vendor/bundle/ruby/2.2.0/gems/rack-1.6.4/lib/rack/utils.rb:489:in
`each’


The error came from this commit:
https://github.com/defunkt/unicorn/commit/fb2f10e1d7a72e6787720003342a21f11b879614.
And specifically the line of `if value =~ /\n/` is changed to `if
value.include?("\n".freeze)`. Apparently `value` can be nil which
caused our issue. It should be an easy fix.


Thanks,

Owen

Re: undefined method `include?' for nil:NilClass (NoMethodError)

[Eric Wong]

Owen Ou <o@heroku.com> wrote:
> We recently upgraded to Unicorn 5.0 but getting the following error:
> 
> [2015-11-16T14:54:16.943652 #19838] ERROR -- : app error: undefined
> method `include?' for nil:NilClass (NoMethodError)
> 
> E, [2015-11-16T14:54:16.943712 #19838] ERROR -- :
> /home/api/vendor/bundle/ruby/2.2.0/gems/unicorn-5.0.0/lib/unicorn/http_response.rb:40:in
> `block in http_response_write'

<snip>

> The error came from this commit:
> https://github.com/defunkt/unicorn/commit/fb2f10e1d7a72e6787720003342a21f11b879614.
> And specifically the line of `if value =~ /\n/` is changed to `if
> value.include?("\n".freeze)`. Apparently `value` can be nil which
> caused our issue. It should be an easy fix.

Yes, easy, I reverted that hunk in the original change since it's
the easiest to verify as correct.  It's unfortunate, change to have
to make, though.

Thanks, will release 5.0.1 in less than a day...

---------------------8<-----------------------
Subject: [PATCH] http_response: allow nil values in response headers

This blatantly violates Rack SPEC, but we've had this bug since
March 2009[1].  Thus, we cannot expect all existing applications
and middlewares to fix this bug and will probably have to
support it forever.

Unfortunately, supporting this bug contributes to application
server lock-in, but at least we'll document it as such.

[1] commit 1835c9e2e12e6674b52dd80e4598cad9c4ea1e84
    ("HttpResponse: speed up non-multivalue headers")

Reported-by: Owen Ou <o@heroku.com>
Ref: <CAO47=rJa=zRcLn_Xm4v2cHPr6c0UswaFC_omYFEH+baSxHOWKQ@mail.gmail.com>
---
  Side note: I don't intend to port this change to less-popular servers
  I maintain.  This bug is yet another example of why monoculture or
  even any sort of majority adoption hurts an ecosystem.

 lib/unicorn/http_response.rb | 2 +-
 test/unit/test_response.rb   | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index c1aa738..7b446c2 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -37,7 +37,7 @@ def http_response_write(socket, status, headers, body,
           # key in Rack < 1.5
           hijack = value
         else
-          if value.include?("\n".freeze)
+          if value =~ /\n/
             # avoiding blank, key-only cookies with /\n+/
             value.split(/\n+/).each { |v| buf << "#{key}: #{v}\r\n" }
           else
diff --git a/test/unit/test_response.rb b/test/unit/test_response.rb
index 0b14d59..fbe433f 100644
--- a/test/unit/test_response.rb
+++ b/test/unit/test_response.rb
@@ -33,6 +33,15 @@ def test_response_headers
     assert out.length > 0, "output didn't have data"
   end
 
+  # ref: <CAO47=rJa=zRcLn_Xm4v2cHPr6c0UswaFC_omYFEH+baSxHOWKQ@mail.gmail.com>
+  def test_response_header_broken_nil
+    out = StringIO.new
+    http_response_write(out, 200, {"Nil" => nil}, %w(hysterical raisin))
+    assert ! out.closed?
+
+    assert_match %r{^Nil: \r\n}sm, out.string, 'nil accepted'
+  end
+
   def test_response_string_status
     out = StringIO.new
     http_response_write(out,'200', {}, [])
-- 
EW