From 084206baceacbf3244c3047af1a51f69b0ec74ff Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 11 Jun 2016 13:31:04 +0900 Subject: [PATCH] nghttpx: Handle edge case wildcard pattern and add tests Suppose the wildcard patterns follows: - *.nghttp2.org/foo - *.img.nghttp2.org/bar Previously, s.img.nghttp2.org/foo does not match anything. Now it matches first pattern. --- src/CMakeLists.txt | 1 + src/Makefile.am | 1 + src/shrpx-unittest.cc | 4 ++ src/shrpx_router.cc | 26 +++++--- src/shrpx_router.h | 11 +++- src/shrpx_router_test.cc | 129 +++++++++++++++++++++++++++++++++++++++ src/shrpx_router_test.h | 39 ++++++++++++ src/shrpx_worker.cc | 28 +++++++-- 8 files changed, 222 insertions(+), 17 deletions(-) create mode 100644 src/shrpx_router_test.cc create mode 100644 src/shrpx_router_test.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index e3822289..41fbb68f 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -149,6 +149,7 @@ if(ENABLE_APP) shrpx_config_test.cc shrpx_worker_test.cc shrpx_http_test.cc + shrpx_router_test.cc http2_test.cc util_test.cc nghttp2_gzip_test.c diff --git a/src/Makefile.am b/src/Makefile.am index 1d671c48..58f5f564 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -175,6 +175,7 @@ nghttpx_unittest_SOURCES = shrpx-unittest.cc \ shrpx_config_test.cc shrpx_config_test.h \ shrpx_worker_test.cc shrpx_worker_test.h \ shrpx_http_test.cc shrpx_http_test.h \ + shrpx_router_test.cc shrpx_router_test.h \ http2_test.cc http2_test.h \ util_test.cc util_test.h \ nghttp2_gzip_test.c nghttp2_gzip_test.h \ diff --git a/src/shrpx-unittest.cc b/src/shrpx-unittest.cc index df6d6b9c..12f7e84e 100644 --- a/src/shrpx-unittest.cc +++ b/src/shrpx-unittest.cc @@ -44,6 +44,7 @@ #include "base64_test.h" #include "shrpx_config.h" #include "ssl.h" +#include "shrpx_router_test.h" static int init_suite1(void) { return 0; } @@ -125,6 +126,9 @@ int main(int argc, char *argv[]) { shrpx::test_shrpx_http_create_forwarded) || !CU_add_test(pSuite, "http_create_via_header_value", shrpx::test_shrpx_http_create_via_header_value) || + !CU_add_test(pSuite, "router_match", shrpx::test_shrpx_router_match) || + !CU_add_test(pSuite, "router_match_prefix", + shrpx::test_shrpx_router_match_prefix) || !CU_add_test(pSuite, "util_streq", shrpx::test_util_streq) || !CU_add_test(pSuite, "util_strieq", shrpx::test_util_strieq) || !CU_add_test(pSuite, "util_inp_strlower", diff --git a/src/shrpx_router.cc b/src/shrpx_router.cc index 7e875aef..caf8383a 100644 --- a/src/shrpx_router.cc +++ b/src/shrpx_router.cc @@ -280,7 +280,7 @@ ssize_t Router::match(const StringRef &host, const StringRef &path) const { } namespace { -const RNode *match_prefix(const RNode *node, const char *first, +const RNode *match_prefix(size_t *nread, const RNode *node, const char *first, const char *last) { if (first == last) { return nullptr; @@ -288,45 +288,53 @@ const RNode *match_prefix(const RNode *node, const char *first, auto p = first; - const RNode *ans = nullptr; - for (;;) { auto next_node = find_next_node(node, *p); if (next_node == nullptr) { - return ans; + return nullptr; } node = next_node; auto n = std::min(node->len, static_cast(last - p)); if (memcmp(node->s, p, n) != 0) { - return ans; + return nullptr; } p += n; if (p != last) { if (node->index != -1) { - ans = node; + *nread = p - first; + return node; } continue; } if (node->len == n) { + *nread = p - first; return node; } - return ans; + return nullptr; } } } // namespace -ssize_t Router::match_prefix(const StringRef &s) const { - auto node = ::shrpx::match_prefix(&root_, std::begin(s), std::end(s)); +ssize_t Router::match_prefix(size_t *nread, const RNode **last_node, + const StringRef &s) const { + if (*last_node == nullptr) { + *last_node = &root_; + } + + auto node = + ::shrpx::match_prefix(nread, *last_node, std::begin(s), std::end(s)); if (node == nullptr) { return -1; } + *last_node = node; + return node->index; } diff --git a/src/shrpx_router.h b/src/shrpx_router.h index 7d3a588e..b193e64a 100644 --- a/src/shrpx_router.h +++ b/src/shrpx_router.h @@ -68,8 +68,15 @@ public: // Returns the matched index of pattern. -1 if there is no match. ssize_t match(const StringRef &host, const StringRef &path) const; // Returns the matched index of pattern if a pattern is a suffix of - // |s|, otherwise -1. - ssize_t match_prefix(const StringRef &s) const; + // |s|, otherwise -1. If |*last_node| is not nullptr, it specifies + // the first node to start matching. If it is nullptr, match will + // start from scratch. When the match was found (the return value + // is not -1), |*nread| has the number of bytes matched in |s|, and + // |*last_node| has the last matched node. One can continue to + // match the longer pattern using the returned |*last_node| to the + // another invocation of this function until it returns -1. + ssize_t match_prefix(size_t *nread, const RNode **last_node, + const StringRef &s) const; void add_node(RNode *node, const char *pattern, size_t patlen, size_t index); diff --git a/src/shrpx_router_test.cc b/src/shrpx_router_test.cc new file mode 100644 index 00000000..0248181c --- /dev/null +++ b/src/shrpx_router_test.cc @@ -0,0 +1,129 @@ +/* + * nghttp2 - HTTP/2 C Library + * + * Copyright (c) 2016 Tatsuhiro Tsujikawa + * + * Permission is hereby granted, free of charge, to any person obtaining + * a copy of this software and associated documentation files (the + * "Software"), to deal in the Software without restriction, including + * without limitation the rights to use, copy, modify, merge, publish, + * distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to + * the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION + * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +#include "shrpx_router_test.h" + +#include + +#include "shrpx_router.h" + +namespace shrpx { + +struct Pattern { + StringRef pattern; + size_t idx; +}; + +void test_shrpx_router_match(void) { + auto patterns = std::vector{ + {StringRef::from_lit("nghttp2.org/"), 0}, + {StringRef::from_lit("nghttp2.org/alpha"), 1}, + {StringRef::from_lit("nghttp2.org/alpha/"), 2}, + {StringRef::from_lit("nghttp2.org/alpha/bravo/"), 3}, + {StringRef::from_lit("www.nghttp2.org/alpha/"), 4}, + {StringRef::from_lit("/alpha"), 5}, + {StringRef::from_lit("example.com/alpha/"), 6}, + }; + + Router router; + + for (auto &p : patterns) { + router.add_route(p.pattern, p.idx); + } + + ssize_t idx; + + idx = router.match(StringRef::from_lit("nghttp2.org"), + StringRef::from_lit("/")); + + CU_ASSERT(0 == idx); + + idx = router.match(StringRef::from_lit("nghttp2.org"), + StringRef::from_lit("/alpha")); + + CU_ASSERT(1 == idx); + + idx = router.match(StringRef::from_lit("nghttp2.org"), + StringRef::from_lit("/alpha/")); + + CU_ASSERT(2 == idx); + + idx = router.match(StringRef::from_lit("nghttp2.org"), + StringRef::from_lit("/alpha/charlie")); + + CU_ASSERT(2 == idx); + + idx = router.match(StringRef::from_lit("nghttp2.org"), + StringRef::from_lit("/alpha/bravo/")); + + CU_ASSERT(3 == idx); + + // matches pattern when last '/' is missing in path + idx = router.match(StringRef::from_lit("nghttp2.org"), + StringRef::from_lit("/alpha/bravo")); + + idx = router.match(StringRef{}, StringRef::from_lit("/alpha")); + + CU_ASSERT(5 == idx); +} + +void test_shrpx_router_match_prefix(void) { + auto patterns = std::vector{ + {StringRef::from_lit("gro.2ptthgn."), 0}, + {StringRef::from_lit("gro.2ptthgn.www."), 1}, + {StringRef::from_lit("gro.2ptthgn.gmi."), 2}, + {StringRef::from_lit("gro.2ptthgn.gmi.ahpla."), 3}, + }; + + Router router; + + for (auto &p : patterns) { + router.add_route(p.pattern, p.idx); + } + + ssize_t idx; + const RNode *node; + size_t nread; + + node = nullptr; + + idx = router.match_prefix(&nread, &node, + StringRef::from_lit("gro.2ptthgn.gmi.ahpla.ovarb")); + + CU_ASSERT(0 == idx); + CU_ASSERT(12 == nread); + + idx = router.match_prefix(&nread, &node, + StringRef::from_lit("gmi.ahpla.ovarb")); + + CU_ASSERT(2 == idx); + CU_ASSERT(4 == nread); + + idx = router.match_prefix(&nread, &node, StringRef::from_lit("ahpla.ovarb")); + + CU_ASSERT(3 == idx); + CU_ASSERT(6 == nread); +} + +} // namespace shrpx diff --git a/src/shrpx_router_test.h b/src/shrpx_router_test.h new file mode 100644 index 00000000..9f4ba665 --- /dev/null +++ b/src/shrpx_router_test.h @@ -0,0 +1,39 @@ +/* + * nghttp2 - HTTP/2 C Library + * + * Copyright (c) 2016 Tatsuhiro Tsujikawa + * + * Permission is hereby granted, free of charge, to any person obtaining + * a copy of this software and associated documentation files (the + * "Software"), to deal in the Software without restriction, including + * without limitation the rights to use, copy, modify, merge, publish, + * distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to + * the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION + * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +#ifndef SHRPX_ROUTER_TEST_H +#define SHRPX_ROUTER_TEST_H + +#ifdef HAVE_CONFIG_H +#include +#endif // HAVE_CONFIG_H + +namespace shrpx { + +void test_shrpx_router_match(void); +void test_shrpx_router_match_prefix(void); + +} // namespace shrpx + +#endif // SHRPX_ROUTER_TEST_H diff --git a/src/shrpx_worker.cc b/src/shrpx_worker.cc index b5651f75..d05d2922 100644 --- a/src/shrpx_worker.cc +++ b/src/shrpx_worker.cc @@ -488,12 +488,23 @@ size_t match_downstream_addr_group_host( } if (!wildcard_patterns.empty() && !host.empty()) { - auto rev_host = std::string{std::begin(host) + 1, std::end(host)}; - std::reverse(std::begin(rev_host), std::end(rev_host)); + auto rev_host_src = std::string{std::begin(host) + 1, std::end(host)}; + std::reverse(std::begin(rev_host_src), std::end(rev_host_src)); + + ssize_t best_group = -1; + const RNode *last_node = nullptr; + auto rev_host = StringRef{std::begin(rev_host_src), std::end(rev_host_src)}; + + for (;;) { + size_t nread = 0; + auto wcidx = + rev_wildcard_router.match_prefix(&nread, &last_node, rev_host); + if (wcidx == -1) { + break; + } + + rev_host = StringRef{std::begin(rev_host) + nread, std::end(rev_host)}; - auto wcidx = rev_wildcard_router.match_prefix( - StringRef{std::begin(rev_host), std::end(rev_host)}); - if (wcidx != -1) { auto &wc = wildcard_patterns[wcidx]; auto group = wc.router.match(StringRef{}, path); if (group != -1) { @@ -503,9 +514,14 @@ size_t match_downstream_addr_group_host( LOG(INFO) << "Found wildcard pattern with query " << host << path << ", matched pattern=" << groups[group]->pattern; } - return group; + + best_group = group; } } + + if (best_group != -1) { + return best_group; + } } group = router.match(StringRef::from_lit(""), path);