Merge pull request #475 from yyamano/issue-473

Fix segmentation fault when set nil to Nginx::Headers_{in,out}
matsumotory · Aug 18, 2020 · 86fc9ba · 86fc9ba
2 parents aee1cdb + 8f37b5f
commit 86fc9ba
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 11 deletions.
diff --git a/src/http/ngx_http_mruby_request.c b/src/http/ngx_http_mruby_request.c
@@ -227,18 +227,28 @@ static ngx_int_t ngx_mrb_set_request_header(mrb_state *mrb, ngx_list_t *headers,
   ngx_http_request_t *r = ngx_mrb_get_request();
 
   key_len = (size_t)RSTRING_LEN(mrb_key);
-  val_len = (size_t)RSTRING_LEN(mrb_val);
-
   key = ngx_pnalloc(pool, key_len);
   if (key == NULL) {
     return NGX_ERROR;
   }
+  ngx_memcpy(key, (u_char *)RSTRING_PTR(mrb_key), key_len);
+
+  /* TODO:optimize later(linear-search is slow) */
+  if (update || mrb_nil_p(mrb_val)) {
+    while (!mrb_nil_p(ngx_mrb_get_request_header(mrb, headers, (char *)key, key_len))) {
+      ngx_mrb_del_request_header(mrb, headers, (char *)key, key_len);
+    }
+  }
+  if (mrb_nil_p(mrb_val)) {
+    /* If the value is nil, just remove the key from the headers. */
+    return NGX_OK;
+  }
+
+  val_len = (size_t)RSTRING_LEN(mrb_val);
   val = ngx_pnalloc(pool, val_len);
   if (val == NULL) {
     return NGX_ERROR;
   }
-
-  ngx_memcpy(key, (u_char *)RSTRING_PTR(mrb_key), key_len);
   ngx_memcpy(val, (u_char *)RSTRING_PTR(mrb_val), val_len);
 
   switch (ngx_mruby_builtin_header_lookup_token(key, key_len)) {
@@ -260,13 +270,6 @@ static ngx_int_t ngx_mrb_set_request_header(mrb_state *mrb, ngx_list_t *headers,
     break;
   }
 
-  /* TODO:optimize later(linear-search is slow) */
-  if (update) {
-    while (!mrb_nil_p(ngx_mrb_get_request_header(mrb, headers, (char *)key, key_len))) {
-      ngx_mrb_del_request_header(mrb, headers, (char *)key, key_len);
-    }
-  }
-
   new_header = ngx_list_push(headers);
   if (new_header == NULL) {
     return NGX_ERROR;

diff --git a/test/conf/nginx.conf b/test/conf/nginx.conf
@@ -936,6 +936,26 @@ http {
               Nginx.rputs h["X-Foo-Bar"]
             ';
         }
+
+        location /issue-473/in {
+            mruby_content_handler_code '
+              h = Nginx::Headers_in.new
+              h["X-Foo"] = nil
+              h["X-Bar"] = nil
+              Nginx.rputs h.all.sort.map {|k,v| "#{k}"}.join(",")
+            ';
+        }
+
+        location /issue-473/out {
+            mruby_content_handler_code '
+              h = Nginx::Headers_out.new
+              h["X-Foo"] = "Foo"
+              h["X-Foo"] = nil
+              h["X-Bar"] = nil
+              h["X-Baz"] = "Baz"
+              Nginx.rputs h.all.sort.map {|k,v| "#{k}"}.join(",")
+            ';
+        }
     }
 
     server {

diff --git a/test/t/ngx_mruby.rb b/test/t/ngx_mruby.rb
@@ -785,6 +785,19 @@ def _run
     res = HttpRequest.new.get base + '/issue-471', nil, {"X-Foo" => "foo", "X-Foo-Bar" => "bar"}
     t.assert_equal 'foobar', res["body"]
   end
+
+  t.assert('ngx_mruby - BUG: header issue 473', 'location /issue-473/in') do
+    res = HttpRequest.new.get base + '/issue-473/in', nil, {'X-Foo' => 'foo'}
+    t.assert_equal 'Accept,Connection,Host', res['body']
+  end
+
+  t.assert('ngx_mruby - BUG: header issue 473', 'location /issue-473/out') do
+    res = HttpRequest.new.get base + '/issue-473/out', nil, {'XX-Foo' => 'foo'}
+    t.assert_equal 'Server,X-Baz,hoge', res['body']
+    t.assert_equal nil, res['x-foo']
+    t.assert_equal nil, res['x-bar']
+    t.assert_equal 'Baz', res['x-baz']
+  end
 end