nghttpx: Make a copy before adding header to Downstream

src/http2.cc

@@ -1793,6 +1793,15 @@ StringRef rewrite_clean_path(BlockAllocator &balloc, const StringRef &src) {
                         StringRef{query, fragment});
 }
 
+StringRef copy_lower(BlockAllocator &balloc, const StringRef &src) {
+  auto iov = make_byte_ref(balloc, src.size() + 1);
+  auto p = iov.base;
+  p = std::copy(std::begin(src), std::end(src), p);
+  *p = '\0';
+  util::inp_strlower(iov.base, p);
+  return StringRef{iov.base, p};
+}
+
 } // namespace http2
 
 } // namespace nghttp2

src/http2.h

@@ -385,6 +385,9 @@ int construct_push_component(BlockAllocator &balloc, StringRef &scheme,
                              StringRef &authority, StringRef &path,
                              const StringRef &base, const StringRef &uri);
 
+// Copies |src| and return its lower-cased version.
+StringRef copy_lower(BlockAllocator &balloc, const StringRef &src);
+
 } // namespace http2
 
 } // namespace nghttp2

src/shrpx-unittest.cc

@@ -101,8 +101,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_add_header_lower",
+      !CU_add_test(pSuite, "downstream_field_store_append_last_header",
-                   shrpx::test_downstream_field_store_add_header_lower) ||
+                   shrpx::test_downstream_field_store_append_last_header) ||
       !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

@@ -348,23 +348,21 @@ void add_header(bool &key_prev, size_t &sum, HeaderRefs &headers,
 }
 } // namespace
 
-namespace {
-void add_header(size_t &sum, HeaderRefs &headers, const StringRef &name,
-                const StringRef &value, bool no_index, int32_t token) {
-  sum += name.size() + value.size();
-  headers.emplace_back(name, value, no_index, token);
-}
-} // namespace
-
 namespace {
 void append_last_header_key(BlockAllocator &balloc, bool &key_prev, size_t &sum,
                             HeaderRefs &headers, const char *data, size_t len) {
   assert(key_prev);
   sum += len;
   auto &item = headers.back();
-  item.name = concat_string_ref(balloc, item.name, StringRef{data, len});
+  auto iov = make_byte_ref(balloc, item.name.size() + len + 1);
-  util::inp_strlower((uint8_t *)item.name.c_str(),
+  auto p = iov.base;
-                     (uint8_t *)item.name.c_str() + item.name.size());
+  p = std::copy(std::begin(item.name), std::end(item.name), p);
+  p = std::copy_n(data, len, p);
+  util::inp_strlower(p - len, p);
+  *p = '\0';
+
+  item.name = StringRef{iov.base, p};
+
   item.token = http2::lookup_token(item.name);
 }
 } // namespace
@@ -424,21 +422,10 @@ const HeaderRefs::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,
-                                  bool no_index) {
-  auto low_name = make_string_ref(balloc_, name);
-  util::inp_strlower((uint8_t *)low_name.c_str(),
-                     (uint8_t *)low_name.c_str() + low_name.size());
-  auto token = http2::lookup_token(low_name);
-  shrpx::add_header(header_key_prev_, buffer_size_, headers_, low_name,
-                    make_string_ref(balloc_, value), no_index, token);
-}
-
 void FieldStore::add_header_token(const StringRef &name, const StringRef &value,
                                   bool no_index, int32_t token) {
-  shrpx::add_header(buffer_size_, headers_, make_string_ref(balloc_, name),
+  shrpx::add_header(header_key_prev_, buffer_size_, headers_, name, value,
-                    make_string_ref(balloc_, value), no_index, token);
+                    no_index, token);
-  make_string_ref(balloc_, name);
 }
 
 void FieldStore::append_last_header_key(const char *data, size_t len) {
@@ -453,23 +440,13 @@ void FieldStore::append_last_header_value(const char *data, size_t len) {
 
 void FieldStore::clear_headers() { headers_.clear(); }
 
-void FieldStore::add_trailer_lower(const StringRef &name,
-                                   const StringRef &value, bool no_index) {
-  auto low_name = make_string_ref(balloc_, name);
-  util::inp_strlower((uint8_t *)low_name.c_str(),
-                     (uint8_t *)low_name.c_str() + low_name.size());
-  auto token = http2::lookup_token(low_name);
-  shrpx::add_header(trailer_key_prev_, buffer_size_, trailers_, low_name,
-                    make_string_ref(balloc_, value), no_index, token);
-}
-
 void FieldStore::add_trailer_token(const StringRef &name,
                                    const StringRef &value, bool no_index,
                                    int32_t token) {
   // Header size limit should be applied to all header and trailer
   // fields combined.
-  shrpx::add_header(buffer_size_, trailers_, make_string_ref(balloc_, name),
+  shrpx::add_header(trailer_key_prev_, buffer_size_, trailers_, name, value,
-                    make_string_ref(balloc_, value), no_index, token);
+                    no_index, token);
 }
 
 void FieldStore::append_last_trailer_key(const char *data, size_t len) {

src/shrpx_downstream.h

@@ -81,8 +81,6 @@ public:
   // such header is found, returns nullptr.
   const HeaderRefs::value_type *header(const StringRef &name) const;
 
-  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, int32_t token);
 
@@ -98,8 +96,6 @@ public:
   // Empties headers.
   void clear_headers();
 
-  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, int32_t token);
 

src/shrpx_downstream_test.cc

@@ -32,35 +32,22 @@
 
 namespace shrpx {
 
-void test_downstream_field_store_add_header_lower(void) {
+void test_downstream_field_store_append_last_header(void) {
   BlockAllocator balloc(4096, 4096);
   FieldStore fs(balloc, 0);
-  fs.add_header_lower(StringRef::from_lit("1"), StringRef::from_lit("0"),
+  fs.add_header_token(StringRef::from_lit("alpha"), StringRef{}, false, -1);
-                      false);
+  auto bravo = StringRef::from_lit("BRAVO");
-  fs.add_header_lower(StringRef::from_lit("2"), StringRef::from_lit("1"),
+  fs.append_last_header_key(bravo.c_str(), bravo.size());
-                      false);
+  auto charlie = StringRef::from_lit("Charlie");
-  fs.add_header_lower(StringRef::from_lit("Charlie"), StringRef::from_lit("2"),
+  fs.append_last_header_value(charlie.c_str(), charlie.size());
-                      false);
+  auto delta = StringRef::from_lit("deltA");
-  fs.add_header_lower(StringRef::from_lit("Alpha"), StringRef::from_lit("3"),
+  fs.append_last_header_value(delta.c_str(), delta.size());
-                      false);
+  fs.add_header_token(StringRef::from_lit("echo"),
-  fs.add_header_lower(StringRef::from_lit("Delta"), StringRef::from_lit("4"),
+                      StringRef::from_lit("foxtrot"), false, -1);
-                      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 =
+  auto ans = HeaderRefs{
-      HeaderRefs{{StringRef::from_lit("1"), StringRef::from_lit("0")},
+      {StringRef::from_lit("alphabravo"), StringRef::from_lit("CharliedeltA")},
-                 {StringRef::from_lit("2"), StringRef::from_lit("1")},
+      {StringRef::from_lit("echo"), StringRef::from_lit("foxtrot")}};
-                 {StringRef::from_lit("charlie"), StringRef::from_lit("2")},
-                 {StringRef::from_lit("alpha"), StringRef::from_lit("3")},
-                 {StringRef::from_lit("delta"), StringRef::from_lit("4")},
-                 {StringRef::from_lit("bravo"), StringRef::from_lit("5")},
-                 {StringRef::from_lit(":method"), StringRef::from_lit("6")},
-                 {StringRef::from_lit(":authority"), StringRef::from_lit("7")}};
   CU_ASSERT(ans == fs.headers());
 }
 

src/shrpx_downstream_test.h

@@ -31,7 +31,7 @@
 
 namespace shrpx {
 
-void test_downstream_field_store_add_header_lower(void);
+void test_downstream_field_store_append_last_header(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

@@ -818,6 +818,7 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
 
   auto &resp = downstream->response();
   auto &httpconf = get_config()->http;
+  auto &balloc = downstream->get_block_allocator();
 
   switch (frame->hd.type) {
   case NGHTTP2_HEADERS: {
@@ -847,13 +848,15 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
 
     if (trailer) {
       // just store header fields for trailer part
-      resp.fs.add_trailer_token(StringRef{name, namelen},
+      resp.fs.add_trailer_token(
-                                StringRef{value, valuelen}, no_index, token);
+          make_string_ref(balloc, StringRef{name, namelen}),
+          make_string_ref(balloc, StringRef{value, valuelen}), no_index, token);
       return 0;
     }
 
-    resp.fs.add_header_token(StringRef{name, namelen},
+    resp.fs.add_header_token(
-                             StringRef{value, valuelen}, no_index, token);
+        make_string_ref(balloc, StringRef{name, namelen}),
+        make_string_ref(balloc, StringRef{value, valuelen}), no_index, token);
     return 0;
   }
   case NGHTTP2_PUSH_PROMISE: {
@@ -870,6 +873,7 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
     assert(promised_downstream);
 
     auto &promised_req = promised_downstream->request();
+    auto &promised_balloc = promised_downstream->get_block_allocator();
 
     // We use request header limit for PUSH_PROMISE
     if (promised_req.fs.buffer_size() + namelen + valuelen >
@@ -886,9 +890,11 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
     }
 
     auto token = http2::lookup_token(name, namelen);
-    promised_req.fs.add_header_token(StringRef{name, namelen},
+    promised_req.fs.add_header_token(
-                                     StringRef{value, valuelen},
+        make_string_ref(promised_balloc, StringRef{name, namelen}),
-                                     flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+        make_string_ref(promised_balloc, StringRef{value, valuelen}),
+        flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
+
     return 0;
   }
   }

src/shrpx_http2_upstream.cc

@@ -176,6 +176,8 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
 
   auto &httpconf = get_config()->http;
 
+  auto &balloc = downstream->get_block_allocator();
+
   if (req.fs.buffer_size() + namelen + valuelen >
           httpconf.request_header_field_buffer ||
       req.fs.num_fields() >= httpconf.max_request_header_fields) {
@@ -206,12 +208,14 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame,
 
   if (frame->headers.cat == NGHTTP2_HCAT_HEADERS) {
     // just store header fields for trailer part
-    req.fs.add_trailer_token(StringRef{name, namelen},
+    req.fs.add_trailer_token(
-                             StringRef{value, valuelen}, no_index, token);
+        make_string_ref(balloc, StringRef{name, namelen}),
+        make_string_ref(balloc, StringRef{value, valuelen}), no_index, token);
     return 0;
   }
 
-  req.fs.add_header_token(StringRef{name, namelen}, StringRef{value, valuelen},
+  req.fs.add_header_token(make_string_ref(balloc, StringRef{name, namelen}),
+                          make_string_ref(balloc, StringRef{value, valuelen}),
                           no_index, token);
   return 0;
 }
@@ -588,27 +592,29 @@ int on_frame_send_callback(nghttp2_session *session, const nghttp2_frame *frame,
 
     for (size_t i = 0; i < frame->push_promise.nvlen; ++i) {
       auto &nv = frame->push_promise.nva[i];
+
+      auto name =
+          make_string_ref(promised_balloc, StringRef{nv.name, nv.namelen});
+      auto value =
+          make_string_ref(promised_balloc, StringRef{nv.value, nv.valuelen});
+
       auto token = http2::lookup_token(nv.name, nv.namelen);
       switch (token) {
       case http2::HD__METHOD:
-        req.method = http2::lookup_method_token(nv.value, nv.valuelen);
+        req.method = http2::lookup_method_token(value);
         break;
       case http2::HD__SCHEME:
-        req.scheme =
+        req.scheme = value;
-            make_string_ref(promised_balloc, StringRef{nv.value, nv.valuelen});
         break;
       case http2::HD__AUTHORITY:
-        req.authority =
+        req.authority = value;
-            make_string_ref(promised_balloc, StringRef{nv.value, nv.valuelen});
         break;
       case http2::HD__PATH:
-        req.path = http2::rewrite_clean_path(promised_balloc,
+        req.path = http2::rewrite_clean_path(promised_balloc, value);
-                                             StringRef{nv.value, nv.valuelen});
         break;
       }
-      req.fs.add_header_token(StringRef{nv.name, nv.namelen},
+      req.fs.add_header_token(name, value, nv.flags & NGHTTP2_NV_FLAG_NO_INDEX,
-                              StringRef{nv.value, nv.valuelen},
+                              token);
-                              nv.flags & NGHTTP2_NV_FLAG_NO_INDEX, token);
     }
 
     promised_downstream->inspect_http2_request();

src/shrpx_http_downstream_connection.cc

@@ -690,6 +690,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) {
   auto downstream = static_cast<Downstream *>(htp->data);
   auto &resp = downstream->response();
   auto &httpconf = get_config()->http;
+  auto &balloc = downstream->get_block_allocator();
 
   if (ensure_header_field_buffer(downstream, httpconf, len) != 0) {
     return -1;
@@ -702,7 +703,9 @@ 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{}, false);
+      auto name = http2::copy_lower(balloc, StringRef{data, len});
+      auto token = http2::lookup_token(name);
+      resp.fs.add_header_token(name, StringRef{}, false, token);
     }
   } else {
     // trailer part
@@ -715,7 +718,9 @@ 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{}, false);
+      auto name = http2::copy_lower(balloc, StringRef{data, len});
+      auto token = http2::lookup_token(name);
+      resp.fs.add_trailer_token(name, StringRef{}, false, token);
     }
   }
   return 0;

src/shrpx_https_upstream.cc

@@ -121,6 +121,7 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) {
   auto downstream = upstream->get_downstream();
   auto &req = downstream->request();
   auto &httpconf = get_config()->http;
+  auto &balloc = downstream->get_block_allocator();
 
   if (req.fs.buffer_size() + len > httpconf.request_header_field_buffer) {
     if (LOG_ENABLED(INFO)) {
@@ -145,7 +146,9 @@ 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{}, false);
+      auto name = http2::copy_lower(balloc, StringRef{data, len});
+      auto token = http2::lookup_token(name);
+      req.fs.add_header_token(name, StringRef{}, false, token);
     }
   } else {
     // trailer part
@@ -159,7 +162,9 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) {
         }
         return -1;
       }
-      req.fs.add_trailer_lower(StringRef{data, len}, StringRef{}, false);
+      auto name = http2::copy_lower(balloc, StringRef{data, len});
+      auto token = http2::lookup_token(name);
+      req.fs.add_trailer_token(name, StringRef{}, false, token);
     }
   }
   return 0;

src/shrpx_mruby_module_request.cc

@@ -213,6 +213,7 @@ mrb_value request_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
   auto data = static_cast<MRubyAssocData *>(mrb->ud);
   auto downstream = data->downstream;
   auto &req = downstream->request();
+  auto &balloc = downstream->get_block_allocator();
 
   check_phase(mrb, data->phase, PHASE_REQUEST);
 
@@ -226,7 +227,8 @@ 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))};
+      make_string_ref(balloc, StringRef{RSTRING_PTR(key),
+                                        static_cast<size_t>(RSTRING_LEN(key))});
   auto token = http2::lookup_token(keyref.byte(), keyref.size());
 
   if (repl) {
@@ -249,15 +251,19 @@ mrb_value request_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
     for (int i = 0; i < n; ++i) {
       auto value = mrb_ary_entry(values, i);
       req.fs.add_header_token(
-          keyref, StringRef{RSTRING_PTR(value),
+          keyref,
-                            static_cast<size_t>(RSTRING_LEN(value))},
+          make_string_ref(balloc,
+                          StringRef{RSTRING_PTR(value),
+                                    static_cast<size_t>(RSTRING_LEN(value))}),
           false, token);
     }
   } else if (!mrb_nil_p(values)) {
-    req.fs.add_header_token(keyref,
+    req.fs.add_header_token(
-                            StringRef{RSTRING_PTR(values),
+        keyref,
-                                      static_cast<size_t>(RSTRING_LEN(values))},
+        make_string_ref(balloc,
-                            false, token);
+                        StringRef{RSTRING_PTR(values),
+                                  static_cast<size_t>(RSTRING_LEN(values))}),
+        false, token);
   }
 
   return mrb_nil_value();

src/shrpx_mruby_module_response.cc

@@ -107,6 +107,7 @@ mrb_value response_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
   auto data = static_cast<MRubyAssocData *>(mrb->ud);
   auto downstream = data->downstream;
   auto &resp = downstream->response();
+  auto &balloc = downstream->get_block_allocator();
 
   mrb_value key, values;
   mrb_get_args(mrb, "oo", &key, &values);
@@ -118,7 +119,8 @@ 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))};
+      make_string_ref(balloc, StringRef{RSTRING_PTR(key),
+                                        static_cast<size_t>(RSTRING_LEN(key))});
   auto token = http2::lookup_token(keyref.byte(), keyref.size());
 
   if (repl) {
@@ -141,14 +143,18 @@ mrb_value response_mod_header(mrb_state *mrb, mrb_value self, bool repl) {
     for (int i = 0; i < n; ++i) {
       auto value = mrb_ary_entry(values, i);
       resp.fs.add_header_token(
-          keyref, StringRef{RSTRING_PTR(value),
+          keyref,
-                            static_cast<size_t>(RSTRING_LEN(value))},
+          make_string_ref(balloc,
+                          StringRef{RSTRING_PTR(value),
+                                    static_cast<size_t>(RSTRING_LEN(value))}),
           false, token);
     }
   } else if (!mrb_nil_p(values)) {
     resp.fs.add_header_token(
-        keyref, StringRef{RSTRING_PTR(values),
+        keyref,
-                          static_cast<size_t>(RSTRING_LEN(values))},
+        make_string_ref(balloc,
+                        StringRef{RSTRING_PTR(values),
+                                  static_cast<size_t>(RSTRING_LEN(values))}),
         false, token);
   }
 
@@ -222,7 +228,6 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) {
   if (cl) {
     cl->value = content_length;
   } else {
-    // TODO we don't have to make a copy here.
     resp.fs.add_header_token(StringRef::from_lit("content-length"),
                              content_length, false, http2::HD_CONTENT_LENGTH);
   }
@@ -232,9 +237,10 @@ 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_token(StringRef::from_lit("date"),
+    resp.fs.add_header_token(
-                             StringRef{lgconf->time_http_str}, false,
+        StringRef::from_lit("date"),
-                             http2::HD_DATE);
+        make_string_ref(balloc, StringRef{lgconf->time_http_str}), false,
+        http2::HD_DATE);
   }
 
   auto upstream = downstream->get_upstream();

src/shrpx_spdy_upstream.cc

@@ -197,7 +197,9 @@ void on_ctrl_recv_callback(spdylay_session *session, spdylay_frame_type type,
       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);
+      req.fs.add_header_token(make_string_ref(balloc, StringRef{name}),
+                              make_string_ref(balloc, StringRef{value}), false,
+                              token);
     }
 
     if (req.fs.parse_content_length() != 0) {