nghttpx: Fix mruby compile error, clean up add_header interface

src/shrpx-unittest.cc

@@ -106,8 +106,8 @@ int main(int argc, char *argv[]) {
                    shrpx::test_http2_get_pure_path_component) ||
       !CU_add_test(pSuite, "http2_construct_push_component",
                    shrpx::test_http2_construct_push_component) ||
-      !CU_add_test(pSuite, "downstream_field_store_index_headers",
-                   shrpx::test_downstream_field_store_index_headers) ||
+      !CU_add_test(pSuite, "downstream_field_store_add_header_lower",
+                   shrpx::test_downstream_field_store_add_header_lower) ||
       !CU_add_test(pSuite, "downstream_field_store_header",
                    shrpx::test_downstream_field_store_header) ||
       !CU_add_test(pSuite, "downstream_crumble_request_cookie",

src/shrpx_downstream.cc

@@ -330,10 +330,10 @@ void Downstream::crumble_request_cookie(std::vector<nghttp2_nv> &nva) {
 
 namespace {
 void add_header(bool &key_prev, size_t &sum, Headers &headers, std::string name,
-                std::string value, int16_t token) {
+                std::string value, bool no_index, int16_t token) {
   key_prev = true;
   sum += name.size() + value.size();
-  headers.emplace_back(std::move(name), std::move(value), false, token);
+  headers.emplace_back(std::move(name), std::move(value), no_index, token);
 }
 } // namespace
 
@@ -411,23 +411,17 @@ const Headers::value_type *FieldStore::header(const StringRef &name) const {
   return search_header_linear_backwards(headers_, name);
 }
 
-void FieldStore::add_header_lower(const StringRef &name,
-                                  const StringRef &value) {
+void FieldStore::add_header_lower(const StringRef &name, const StringRef &value,
+                                  bool no_index) {
   auto low_name = name.str();
   util::inp_strlower(low_name);
   auto token = http2::lookup_token(low_name);
   shrpx::add_header(header_key_prev_, buffer_size_, headers_,
-                    std::move(low_name), value.str(), token);
+                    std::move(low_name), value.str(), no_index, token);
 }
 
-void FieldStore::add_header(std::string name, std::string value,
-                            int16_t token) {
-  buffer_size_ += name.size() + value.size();
-  headers_.emplace_back(std::move(name), std::move(value), false, token);
-}
-
-void FieldStore::add_header(const StringRef &name, const StringRef &value,
-                            bool no_index, int16_t token) {
+void FieldStore::add_header_token(const StringRef &name, const StringRef &value,
+                                  bool no_index, int16_t token) {
   shrpx::add_header(buffer_size_, headers_, name, value, no_index, token);
 }
 
@@ -443,20 +437,21 @@ void FieldStore::append_last_header_value(const char *data, size_t len) {
 
 void FieldStore::clear_headers() { headers_.clear(); }
 
-void FieldStore::add_trailer(const StringRef &name, const StringRef &value,
-                             bool no_index, int16_t token) {
-  // Header size limit should be applied to all header and trailer
-  // fields combined.
-  shrpx::add_header(buffer_size_, trailers_, name, value, no_index, token);
-}
-
 void FieldStore::add_trailer_lower(const StringRef &name,
-                                   const StringRef &value) {
+                                   const StringRef &value, bool no_index) {
   auto low_name = name.str();
   util::inp_strlower(low_name);
   auto token = http2::lookup_token(low_name);
   shrpx::add_header(trailer_key_prev_, buffer_size_, trailers_,
-                    std::move(low_name), value.str(), token);
+                    std::move(low_name), value.str(), no_index, token);
+}
+
+void FieldStore::add_trailer_token(const StringRef &name,
+                                   const StringRef &value, bool no_index,
+                                   int16_t token) {
+  // Header size limit should be applied to all header and trailer
+  // fields combined.
+  shrpx::add_header(buffer_size_, trailers_, name, value, no_index, token);
 }
 
 void FieldStore::append_last_trailer_key(const char *data, size_t len) {

src/shrpx_downstream.h

@@ -79,10 +79,10 @@ public:
   // such header is found, returns nullptr.
   const Headers::value_type *header(const StringRef &name) const;
 
-  void add_header_lower(const StringRef &name, const StringRef &value);
-  void add_header(std::string name, std::string value, int16_t token);
-  void add_header(const StringRef &name, const StringRef &value, bool no_index,
-                  int16_t token);
+  void add_header_lower(const StringRef &name, const StringRef &value,
+                        bool no_index);
+  void add_header_token(const StringRef &name, const StringRef &value,
+                        bool no_index, int16_t token);
 
   void append_last_header_key(const char *data, size_t len);
   void append_last_header_value(const char *data, size_t len);
@@ -97,9 +97,10 @@ public:
   // Empties headers.
   void clear_headers();
 
-  void add_trailer(const StringRef &name, const StringRef &value, bool no_index,
-                   int16_t token);
-  void add_trailer_lower(const StringRef &name, const StringRef &value);
+  void add_trailer_lower(const StringRef &name, const StringRef &value,
+                         bool no_index);
+  void add_trailer_token(const StringRef &name, const StringRef &value,
+                         bool no_index, int16_t token);
 
   void append_last_trailer_key(const char *data, size_t len);
   void append_last_trailer_value(const char *data, size_t len);

src/shrpx_downstream_test.cc

@@ -32,17 +32,24 @@
 
 namespace shrpx {
 
-void test_downstream_field_store_index_headers(void) {
+void test_downstream_field_store_add_header_lower(void) {
   FieldStore fs(0);
-  fs.add_header("1", "0");
-  fs.add_header("2", "1");
-  fs.add_header("Charlie", "2");
-  fs.add_header("Alpha", "3");
-  fs.add_header("Delta", "4");
-  fs.add_header("BravO", "5");
-  fs.add_header(":method", "6");
-  fs.add_header(":authority", "7");
-  fs.index_headers();
+  fs.add_header_lower(StringRef::from_lit("1"), StringRef::from_lit("0"),
+                      false);
+  fs.add_header_lower(StringRef::from_lit("2"), StringRef::from_lit("1"),
+                      false);
+  fs.add_header_lower(StringRef::from_lit("Charlie"), StringRef::from_lit("2"),
+                      false);
+  fs.add_header_lower(StringRef::from_lit("Alpha"), StringRef::from_lit("3"),
+                      false);
+  fs.add_header_lower(StringRef::from_lit("Delta"), StringRef::from_lit("4"),
+                      false);
+  fs.add_header_lower(StringRef::from_lit("BravO"), StringRef::from_lit("5"),
+                      false);
+  fs.add_header_lower(StringRef::from_lit(":method"), StringRef::from_lit("6"),
+                      false);
+  fs.add_header_lower(StringRef::from_lit(":authority"),
+                      StringRef::from_lit("7"), false);
 
   auto ans = Headers{{"1", "0"},
                      {"2", "1"},
@@ -57,10 +64,13 @@ void test_downstream_field_store_index_headers(void) {
 
 void test_downstream_field_store_header(void) {
   FieldStore fs(0);
-  fs.add_header("alpha", "0");
-  fs.add_header(":authority", "1");
-  fs.add_header("content-length", "2");
-  fs.index_headers();
+  fs.add_header_token(StringRef::from_lit("alpha"), StringRef::from_lit("0"),
+                      false, -1);
+  fs.add_header_token(StringRef::from_lit(":authority"),
+                      StringRef::from_lit("1"), false, http2::HD__AUTHORITY);
+  fs.add_header_token(StringRef::from_lit("content-length"),
+                      StringRef::from_lit("2"), false,
+                      http2::HD_CONTENT_LENGTH);
 
   // By token
   CU_ASSERT(Header(":authority", "1") == *fs.header(http2::HD__AUTHORITY));
@@ -74,14 +84,18 @@ void test_downstream_field_store_header(void) {
 void test_downstream_crumble_request_cookie(void) {
   Downstream d(nullptr, nullptr, 0);
   auto &req = d.request();
-  req.fs.add_header(":method", "get");
-  req.fs.add_header(":path", "/");
-  auto val = "alpha; bravo; ; ;; charlie;;";
-  req.fs.add_header(
-      reinterpret_cast<const uint8_t *>("cookie"), sizeof("cookie") - 1,
-      reinterpret_cast<const uint8_t *>(val), strlen(val), true, -1);
-  req.fs.add_header("cookie", ";delta");
-  req.fs.add_header("cookie", "echo");
+  req.fs.add_header_token(StringRef::from_lit(":method"),
+                          StringRef::from_lit("get"), false, -1);
+  req.fs.add_header_token(StringRef::from_lit(":path"),
+                          StringRef::from_lit("/"), false, -1);
+  req.fs.add_header_token(StringRef::from_lit("cookie"),
+                          StringRef::from_lit("alpha; bravo; ; ;; charlie;;"),
+                          true, http2::HD_COOKIE);
+  req.fs.add_header_token(StringRef::from_lit("cookie"),
+                          StringRef::from_lit(";delta"), false,
+                          http2::HD_COOKIE);
+  req.fs.add_header_token(StringRef::from_lit("cookie"),
+                          StringRef::from_lit("echo"), false, http2::HD_COOKIE);
 
   std::vector<nghttp2_nv> nva;
   d.crumble_request_cookie(nva);
@@ -114,12 +128,22 @@ void test_downstream_crumble_request_cookie(void) {
 void test_downstream_assemble_request_cookie(void) {
   Downstream d(nullptr, nullptr, 0);
   auto &req = d.request();
-  req.fs.add_header(":method", "get");
-  req.fs.add_header(":path", "/");
-  req.fs.add_header("cookie", "alpha");
-  req.fs.add_header("cookie", "bravo;");
-  req.fs.add_header("cookie", "charlie; ");
-  req.fs.add_header("cookie", "delta;;");
+  req.fs.add_header_token(StringRef::from_lit(":method"),
+                          StringRef::from_lit("get"), false, -1);
+  req.fs.add_header_token(StringRef::from_lit(":path"),
+                          StringRef::from_lit("/"), false, -1);
+  req.fs.add_header_token(StringRef::from_lit("cookie"),
+                          StringRef::from_lit("alpha"), false,
+                          http2::HD_COOKIE);
+  req.fs.add_header_token(StringRef::from_lit("cookie"),
+                          StringRef::from_lit("bravo;"), false,
+                          http2::HD_COOKIE);
+  req.fs.add_header_token(StringRef::from_lit("cookie"),
+                          StringRef::from_lit("charlie; "), false,
+                          http2::HD_COOKIE);
+  req.fs.add_header_token(StringRef::from_lit("cookie"),
+                          StringRef::from_lit("delta;;"), false,
+                          http2::HD_COOKIE);
   CU_ASSERT("alpha; bravo; charlie; delta" == d.assemble_request_cookie());
 }
 
@@ -129,8 +153,9 @@ void test_downstream_rewrite_location_response_header(void) {
   auto &resp = d.response();
   d.set_request_downstream_host("localhost2");
   req.authority = "localhost:8443";
-  resp.fs.add_header("location", "http://localhost2:3000/");
-  resp.fs.index_headers();
+  resp.fs.add_header_token(StringRef::from_lit("location"),
+                           StringRef::from_lit("http://localhost2:3000/"),
+                           false, http2::HD_LOCATION);
   d.rewrite_location_response_header("https");
   auto location = resp.fs.header(http2::HD_LOCATION);
   CU_ASSERT("https://localhost:8443/" == (*location).value);

src/shrpx_downstream_test.h

@@ -31,7 +31,7 @@
 
 namespace shrpx {
 
-void test_downstream_field_store_index_headers(void);
+void test_downstream_field_store_add_header_lower(void);
 void test_downstream_field_store_header(void);
 void test_downstream_crumble_request_cookie(void);
 void test_downstream_assemble_request_cookie(void);

src/shrpx_http2_session.cc

@@ -736,16 +736,17 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
     }
 
     auto token = http2::lookup_token(name, namelen);
+    auto no_index = flags & NGHTTP2_NV_FLAG_NO_INDEX;
 
     if (trailer) {
       // just store header fields for trailer part
-      resp.fs.add_trailer(StringRef{name, namelen}, StringRef{value, valuelen},
-                          flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+      resp.fs.add_trailer_token(StringRef{name, namelen},
+                                StringRef{value, valuelen}, no_index, token);
       return 0;
     }
 
-    resp.fs.add_header(StringRef{name, namelen}, StringRef{value, valuelen},
-                       flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+    resp.fs.add_header_token(StringRef{name, namelen},
+                             StringRef{value, valuelen}, no_index, token);
     return 0;
   }
   case NGHTTP2_PUSH_PROMISE: {
@@ -778,9 +779,9 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
     }
 
     auto token = http2::lookup_token(name, namelen);
-    promised_req.fs.add_header(StringRef{name, namelen},
-                               StringRef{value, valuelen},
-                               flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+    promised_req.fs.add_header_token(StringRef{name, namelen},
+                                     StringRef{value, valuelen},
+                                     flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
     return 0;
   }
   }
@@ -927,8 +928,9 @@ int on_response_headers(Http2Session *http2session, Downstream *downstream,
         // Otherwise, use chunked encoding to keep upstream connection
         // open.  In HTTP2, we are supporsed not to receive
         // transfer-encoding.
-        resp.fs.add_header("transfer-encoding", "chunked",
-                           http2::HD_TRANSFER_ENCODING);
+        resp.fs.add_header_token(StringRef::from_lit("transfer-encoding"),
+                                 StringRef::from_lit("chunked"), false,
+                                 http2::HD_TRANSFER_ENCODING);
         downstream->set_chunked_response(true);
       }
     }

src/shrpx_http2_upstream.cc

@@ -202,16 +202,17 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
   }
 
   auto token = http2::lookup_token(name, namelen);
+  auto no_index = flags & NGHTTP2_NV_FLAG_NO_INDEX;
 
   if (frame->headers.cat == NGHTTP2_HCAT_HEADERS) {
     // just store header fields for trailer part
-    req.fs.add_trailer(StringRef{name, namelen}, StringRef{value, valuelen},
-                       flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+    req.fs.add_trailer_token(StringRef{name, namelen},
+                             StringRef{value, valuelen}, no_index, token);
     return 0;
   }
 
-  req.fs.add_header(StringRef{name, namelen}, StringRef{value, valuelen},
-                    flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+  req.fs.add_header_token(StringRef{name, namelen}, StringRef{value, valuelen},
+                          no_index, token);
   return 0;
 }
 } // namespace
@@ -593,9 +594,9 @@ int on_frame_send_callback(nghttp2_session *session, const nghttp2_frame *frame,
         req.path = http2::rewrite_clean_path(nv.value, nv.value + nv.valuelen);
         break;
       }
-      req.fs.add_header(StringRef{nv.name, nv.namelen},
-                        StringRef{nv.value, nv.valuelen},
-                        nv.flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+      req.fs.add_header_token(StringRef{nv.name, nv.namelen},
+                              StringRef{nv.value, nv.valuelen},
+                              nv.flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
     }
 
     promised_downstream->inspect_http2_request();

src/shrpx_http_downstream_connection.cc

@@ -691,7 +691,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) {
       if (ensure_max_header_fields(downstream, httpconf) != 0) {
         return -1;
       }
-      resp.fs.add_header_lower(StringRef{data, len}, StringRef{});
+      resp.fs.add_header_lower(StringRef{data, len}, StringRef{}, false);
     }
   } else {
     // trailer part
@@ -704,7 +704,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) {
         // wrong place or crash if trailer fields are currently empty.
         return -1;
       }
-      resp.fs.add_trailer_lower(StringRef(data, len), StringRef{});
+      resp.fs.add_trailer_lower(StringRef(data, len), StringRef{}, false);
     }
   }
   return 0;

src/shrpx_https_upstream.cc

@@ -142,7 +142,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) {
             Downstream::HTTP1_REQUEST_HEADER_TOO_LARGE);
         return -1;
       }
-      req.fs.add_header_lower(StringRef{data, len}, StringRef{});
+      req.fs.add_header_lower(StringRef{data, len}, StringRef{}, false);
     }
   } else {
     // trailer part
@@ -156,7 +156,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) {
         }
         return -1;
       }
-      req.fs.add_trailer_lower(StringRef{data, len}, StringRef{});
+      req.fs.add_trailer_lower(StringRef{data, len}, StringRef{}, false);
     }
   }
   return 0;

src/shrpx_mruby.cc

@@ -94,14 +94,6 @@ int MRubyContext::run_app(Downstream *downstream, int phase) {
 
   mrb_->ud = nullptr;
 
-  if (data.request_headers_dirty) {
-    downstream->request().fs.index_headers();
-  }
-
-  if (data.response_headers_dirty) {
-    downstream->response().fs.index_headers();
-  }
-
   return rv;
 }
 

src/shrpx_mruby.h

@@ -67,8 +67,6 @@ enum {
 struct MRubyAssocData {
   Downstream *downstream;
   int phase;
-  bool request_headers_dirty;
-  bool response_headers_dirty;
 };
 
 RProc *compile(mrb_state *mrb, const StringRef &filename);

src/shrpx_mruby_module_request.cc

@@ -216,17 +216,20 @@ mrb_value request_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
 
   key = mrb_funcall(mrb, key, "downcase", 0);
 
+  auto keyref =
+      StringRef{RSTRING_PTR(key), static_cast<size_t>(RSTRING_LEN(key))};
+  auto token = http2::lookup_token(keyref.byte(), keyref.size());
+
   if (repl) {
     size_t p = 0;
     auto &headers = req.fs.headers();
     for (size_t i = 0; i < headers.size(); ++i) {
-      auto &hd = headers[i];
-      if (util::streq(std::begin(hd.name), hd.name.size(), RSTRING_PTR(key),
-                      RSTRING_LEN(key))) {
+      auto &kv = headers[i];
+      if (kv.name == keyref) {
         continue;
       }
       if (i != p) {
-        headers[p++] = std::move(hd);
+        headers[p++] = std::move(kv);
       }
     }
     headers.resize(p);
@@ -236,16 +239,18 @@ mrb_value request_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
     auto n = mrb_ary_len(mrb, values);
     for (int i = 0; i < n; ++i) {
       auto value = mrb_ary_entry(values, i);
-      req.fs.add_header(std::string(RSTRING_PTR(key), RSTRING_LEN(key)),
-                        std::string(RSTRING_PTR(value), RSTRING_LEN(value)));
+      req.fs.add_header_token(
+          keyref, StringRef{RSTRING_PTR(value),
+                            static_cast<size_t>(RSTRING_LEN(value))},
+          false, token);
     }
   } else if (!mrb_nil_p(values)) {
-    req.fs.add_header(std::string(RSTRING_PTR(key), RSTRING_LEN(key)),
-                      std::string(RSTRING_PTR(values), RSTRING_LEN(values)));
+    req.fs.add_header_token(keyref,
+                            StringRef{RSTRING_PTR(values),
+                                      static_cast<size_t>(RSTRING_LEN(values))},
+                            false, token);
   }
 
-  data->request_headers_dirty = true;
-
   return mrb_nil_value();
 }
 } // namespace

src/shrpx_mruby_module_response.cc

@@ -117,17 +117,20 @@ mrb_value response_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
 
   key = mrb_funcall(mrb, key, "downcase", 0);
 
+  auto keyref =
+      StringRef{RSTRING_PTR(key), static_cast<size_t>(RSTRING_LEN(key))};
+  auto token = http2::lookup_token(keyref.byte(), keyref.size());
+
   if (repl) {
     size_t p = 0;
     auto &headers = resp.fs.headers();
     for (size_t i = 0; i < headers.size(); ++i) {
-      auto &hd = headers[i];
-      if (util::streq(std::begin(hd.name), hd.name.size(), RSTRING_PTR(key),
-                      RSTRING_LEN(key))) {
+      auto &kv = headers[i];
+      if (kv.name == keyref) {
         continue;
       }
       if (i != p) {
-        headers[p++] = std::move(hd);
+        headers[p++] = std::move(kv);
       }
     }
     headers.resize(p);
@@ -137,16 +140,18 @@ mrb_value response_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
     auto n = mrb_ary_len(mrb, values);
     for (int i = 0; i < n; ++i) {
       auto value = mrb_ary_entry(values, i);
-      resp.fs.add_header(std::string(RSTRING_PTR(key), RSTRING_LEN(key)),
-                         std::string(RSTRING_PTR(value), RSTRING_LEN(value)));
+      resp.fs.add_header_token(
+          keyref, StringRef{RSTRING_PTR(value),
+                            static_cast<size_t>(RSTRING_LEN(value))},
+          false, token);
     }
   } else if (!mrb_nil_p(values)) {
-    resp.fs.add_header(std::string(RSTRING_PTR(key), RSTRING_LEN(key)),
-                       std::string(RSTRING_PTR(values), RSTRING_LEN(values)));
+    resp.fs.add_header_token(
+        keyref, StringRef{RSTRING_PTR(values),
+                          static_cast<size_t>(RSTRING_LEN(values))},
+        false, token);
   }
 
-  data->response_headers_dirty = true;
-
   return mrb_nil_value();
 }
 } // namespace
@@ -197,11 +202,6 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) {
     resp.http_status = 200;
   }
 
-  if (data->response_headers_dirty) {
-    resp.fs.index_headers();
-    data->response_headers_dirty = false;
-  }
-
   if (downstream->expect_response_body() && vallen > 0) {
     body = reinterpret_cast<const uint8_t *>(val);
     bodylen = vallen;
@@ -211,8 +211,9 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) {
   if (cl) {
     cl->value = util::utos(bodylen);
   } else {
-    resp.fs.add_header("content-length", util::utos(bodylen),
-                       http2::HD_CONTENT_LENGTH);
+    resp.fs.add_header_token(StringRef::from_lit("content-length"),
+                             StringRef{util::utos(bodylen)}, false,
+                             http2::HD_CONTENT_LENGTH);
   }
   resp.fs.content_length = bodylen;
 
@@ -220,7 +221,9 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) {
   if (!date) {
     auto lgconf = log_config();
     lgconf->update_tstamp(std::chrono::system_clock::now());
-    resp.fs.add_header("date", lgconf->time_http_str, http2::HD_DATE);
+    resp.fs.add_header_token(StringRef::from_lit("date"),
+                             StringRef{lgconf->time_http_str}, false,
+                             http2::HD_DATE);
   }
 
   auto upstream = downstream->get_upstream();

src/shrpx_spdy_upstream.cc

@@ -188,10 +188,10 @@ void on_ctrl_recv_callback(spdylay_session *session, spdylay_frame_type type,
     }
 
     for (size_t i = 0; nv[i]; i += 2) {
-      auto name = std::string(nv[i]);
-      auto value = std::string(nv[i + 1]);
-      auto token = http2::lookup_token(name);
-      req.fs.add_header(std::move(name), std::move(value), token);
+      auto name = StringRef{nv[i]};
+      auto value = StringRef{nv[i + 1]};
+      auto token = http2::lookup_token(name.byte(), name.size());
+      req.fs.add_header_token(name, value, false, token);
     }
 
     if (req.fs.index_headers() != 0) {