Various fixes and improvements.
authorbmcquade <bmcquade@google.com>
Tue, 2 Aug 2011 21:59:25 +0000 (21:59 +0000)
committerbmcquade <bmcquade@google.com>
Tue, 2 Aug 2011 21:59:25 +0000 (21:59 +0000)
src/domain_registry/domain_registry.gyp
src/domain_registry/domain_registry.h
src/domain_registry/domain_registry_test.cc
src/domain_registry/private/assert.c
src/domain_registry/private/registry_search.c
src/domain_registry/private/registry_search_test.cc
src/domain_registry/private/string_util.h
src/domain_registry/private/string_util_test.cc
src/domain_registry/testing/simple_node_table.c

index dd2b843..20e4362 100644 (file)
@@ -81,6 +81,7 @@
       'type': 'executable',
       'dependencies': [
         '../registry_tables_generator/registry_tables_generator.gyp:generate_registry_tables',
+        'assert_lib',
         'domain_registry_lib',
         'init_registry_tables_lib',
         '<(DEPTH)/testing/gtest.gyp:gtest',
index bc62948..e13a05d 100644 (file)
@@ -29,12 +29,12 @@ void InitializeDomainRegistry(void);
 // Finds the length in bytes of the registrar portion of the host in
 // the given hostname.  Returns 0 if the hostname is invalid or has no
 // host (e.g. a file: URL). Returns 0 if the hostname has multiple
-// trailing dots, is an IP address, has no subcomponents, or is itself
-// a recognized registry identifier. If no matching rule is found in
-// the effective-TLD data, returns 0. Internationalized domain names
-// (IDNs) must be converted to punycode before being processed.
-// Non-ASCII hostnames or hostnames longer than 127 bytes will
-// return 0.
+// trailing dots. If no matching rule is found in the effective-TLD
+// data, returns 0. Internationalized domain names (IDNs) must be
+// converted to punycode first. Non-ASCII hostnames or hostnames
+// longer than 127 bytes will return 0. It is an error to pass in an
+// IP address (either IPv4 or IPv6) and the return value in this case
+// is undefined.
 //
 // Examples:
 //   www.google.com       -> 3                 (com)
@@ -45,19 +45,21 @@ void InitializeDomainRegistry(void);
 //   a.b.co..uk           -> 0                 (multiple dots in registry)
 //   C:                   -> 0                 (no host)
 //   google.com..         -> 0                 (multiple trailing dots)
-//   192.168.0.1          -> 0                 (IP address)
 //   bar                  -> 0                 (no subcomponents)
-//   co.uk                -> 0                 (host is a registry)
+//   co.uk                -> 5                 (co.uk)
 //   foo.bar              -> 0                 (not a valid top-level registry)
 //   foo.臺灣               -> 0                 (not converted to punycode)
 //   foo.xn--nnx388a      -> 11                (punycode representation of 臺灣)
+//   192.168.0.1          -> ?                 (IP address, retval undefined)
 size_t GetRegistryLength(const char* hostname);
 
 // Like GetRegistryLength, but allows unknown registries as well. If
 // the hostname is part of a known registry, the return value will be
 // identical to that of GetRegistryLength. If the hostname is not part
 // of a known registry (e.g. foo.bar) then the return value will
-// assume that the rootmost hostname-part is the registry.
+// assume that the rootmost hostname-part is the registry.  It is an
+// error to pass in an IP address (either IPv4 or IPv6) and the return
+// value in this case is undefined.
 //
 // Examples:
 //   foo.bar              -> 3                 (bar)
@@ -65,8 +67,24 @@ size_t GetRegistryLength(const char* hostname);
 //   www.google.com       -> 3                 (com)
 //   com                  -> 0                 (host is a registry)
 //   co.uk                -> 0                 (host is a registry)
+//   foo.臺灣               -> 0                 (not converted to punycode)
+//   foo.xn--nnx388a      -> 11                (punycode representation of 臺灣)
+//   192.168.0.1          -> ?                 (IP address, retval undefined)
 size_t GetRegistryLengthAllowUnknownRegistries(const char* hostname);
 
+// Override the assertion handler by providing a custom assert handler
+// implementation. The assertion handler will be invoked when an
+// internal assertion fails. This is usually indicative of a fatal
+// error and execution should not be allowed to continue. The default
+// implementation logs the assertion information to stderr and invokes
+// abort(). The parameter "file" is the filename where the assertion
+// was triggered. "line_number" is the line number in that
+// file. "cond_str" is the string representation of the assertion that
+// failed.
+typedef void (*DomainRegistryAssertHandler)(
+    const char* file, int line_number, const char* cond_str);
+void SetDomainRegistryAssertHandler(DomainRegistryAssertHandler handler);
+
 #ifdef __cplusplus
 }  // extern "C"
 #endif
index 1f8ee56..7c6edc3 100644 (file)
 #include "domain_registry/domain_registry.h"
 #include "domain_registry/testing/test_entry.h"
 
+extern "C" {
+#include "domain_registry/private/assert.h"
+}  // extern "C"
+
 #include "testing/gtest/include/gtest/gtest.h"
 
 // Include the generated file that contains the actual registry tables.
@@ -42,12 +46,40 @@ TEST_F(DomainRegistryTest, All) {
     // Get the substring that represents the actual registry.
     const char* registry = hostname + strlen(hostname) - actual_registry_len;
     EXPECT_EQ('.', *(registry - 1)) << hostname;
+    if (test_entry->is_exception_rule != 0) {
+      // For exception rules, the returned registry length isn't the
+      // same as the length of the registry. For instance if we have
+      // rule !foo.bar and receive input www.foo.bar, we would expect
+      // a registry length of 3. However, passing in just that
+      // registry ("bar") would no longer trigger the exception
+      // rule. Instead we need to pass in the registry plus the
+      // exception component (e.g. "foo.bar"). To find
+      // "foo.bar" we need to search backwards to the previous dot.
+
+      // First verify that passing in just the registry (e.g. "bar")
+      // produces a registry length of 0:
+      EXPECT_EQ(0, GetRegistryLength(registry)) << hostname << ", " << registry;
+
+      // Skip over the dot that precedes the registry before beginning
+      // to search for the next dot.
+      registry -= 2;
+
+      // Walk backwards until we find the next dot.
+      while (*registry != '.') {
+        EXPECT_GT(registry, hostname) << hostname << ", " << registry;
+        --registry;
+      }
+      // The registry starts immediately after the dot.
+      ++registry;
+
+      EXPECT_EQ('.', *(registry - 1)) << hostname << ", " << registry;
+    }
 
     // When we pass in just the registry and ask for the registry
-    // length, we expect to always get length 0, which indicates that
-    // there is not a match.
+    // length, we expect to get the registry length.
     actual_registry_len = GetRegistryLength(registry);
-    EXPECT_EQ(0, actual_registry_len) << hostname;
+    EXPECT_EQ(expected_registry_len, actual_registry_len)
+        << hostname << ", " << registry;
   }
 }
 
@@ -60,21 +92,83 @@ TEST_F(DomainRegistryTest, Examples) {
   EXPECT_EQ(0, GetRegistryLength("a.b.co..uk"));
   EXPECT_EQ(0, GetRegistryLength("C:"));
   EXPECT_EQ(0, GetRegistryLength("google.com.."));
-  EXPECT_EQ(0, GetRegistryLength("192.168.0.1"));
   EXPECT_EQ(0, GetRegistryLength("bar"));
-  EXPECT_EQ(0, GetRegistryLength("co.uk"));
+  EXPECT_EQ(5, GetRegistryLength("co.uk"));
   EXPECT_EQ(0, GetRegistryLength("foo.bar"));
-  EXPECT_EQ(0, GetRegistryLength("bar"));
   EXPECT_EQ(0, GetRegistryLength("foo.臺灣"));
   EXPECT_EQ(11, GetRegistryLength("foo.xn--nnx388a"));
 
   EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("foo.bar"));
-  EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries("bar"));
+  EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("bar"));
   EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("www.google.com"));
-  EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries("com"));
-  EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries("co.uk"));
+  EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("com"));
+  EXPECT_EQ(5, GetRegistryLengthAllowUnknownRegistries("co.uk"));
   EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries("foo.臺灣"));
   EXPECT_EQ(11, GetRegistryLengthAllowUnknownRegistries("foo.xn--nnx388a"));
+
+  // It is an error to pass in an IP address but we include some tests
+  // to verify that they do not cause crashes. The actual return
+  // values are not specified so we don't expect specific values for
+  // these calls.
+  GetRegistryLength("192.168.0.1");
+  GetRegistryLength("2001:0db8:85a3:0000:0000:8a2e:0370:7334");
+  GetRegistryLengthAllowUnknownRegistries("192.168.0.1");
+  GetRegistryLengthAllowUnknownRegistries(
+      "2001:0db8:85a3:0000:0000:8a2e:0370:7334");
+}
+
+class AssertHandlerTest : public ::testing::Test {
+ protected:
+  static void TestAssertHandler(
+      const char* file, int line, const char* cond_str) {
+    g_assert_file = file;
+    g_assert_line = line;
+    g_assert_cond_str = cond_str;
+  }
+
+  virtual void SetUp() {
+    ClearAssertion();
+  }
+
+  void ClearAssertion() {
+    g_assert_file = NULL;
+    g_assert_line = -1;
+    g_assert_cond_str = NULL;
+  }
+
+  void AssertNoAssertion() {
+    AssertAssertion(NULL, -1, NULL);
+  }
+
+  void AssertAssertion(
+      const char* file, int line, const char* cond_str) {
+    ASSERT_EQ(file, g_assert_file);
+    ASSERT_EQ(line, g_assert_line);
+    ASSERT_EQ(cond_str, g_assert_cond_str);
+  }
+
+  static const char* g_assert_file;
+  static int g_assert_line;
+  static const char* g_assert_cond_str;
+
+  static const char* const kFilename;
+  static const char* const kCondStr;
+};
+
+const char* AssertHandlerTest::g_assert_file = NULL;
+int AssertHandlerTest::g_assert_line = -1;
+const char* AssertHandlerTest::g_assert_cond_str = NULL;
+
+// dummy values
+const char* const AssertHandlerTest::kFilename = "filename";
+const char* const AssertHandlerTest::kCondStr = "cond str";
+
+TEST_F(AssertHandlerTest, OverrideAssertHandler) {
+  SetDomainRegistryAssertHandler(TestAssertHandler);
+  DoAssert(kFilename, 1, kCondStr, 1);
+  AssertNoAssertion();
+  DoAssert(kFilename, 1, kCondStr, 0);
+  AssertAssertion(kFilename, 1, kCondStr);
 }
 
 }  // namespace
index 3560b07..48634ca 100644 (file)
 
 #include "domain_registry/private/assert.h"
 
+#include "domain_registry/domain_registry.h"
 #include <stdio.h>
 #include <stdlib.h>
 
+void DefaultAssertHandler(const char* file, int line, const char* cond_str) {
+    fprintf(stderr, "%s:%d. CHECK failed: %s\n", file, line, cond_str);
+    abort();
+}
+
+static DomainRegistryAssertHandler g_assert_hander = DefaultAssertHandler;
+
 void DoAssert(const char* file,
               int line,
               const char* condition_str,
               int condition) {
   if (condition == 0) {
-    fprintf(stderr, "%s:%d. CHECK failed: %s\n", file, line, condition_str);
-    abort();
+    g_assert_hander(file, line, condition_str);
   }
 }
 
+void SetDomainRegistryAssertHandler(DomainRegistryAssertHandler handler) {
+  g_assert_hander = handler;
+}
index f0ed445..1889707 100644 (file)
@@ -173,7 +173,6 @@ static size_t GetRegistryLengthImpl(
     const char* value_end,
     const char sep,
     int allow_unknown_registries) {
-  const size_t value_len = value_end - value;
   const char* registry;
   size_t match_len;
 
@@ -202,13 +201,6 @@ static size_t GetRegistryLengthImpl(
       return 0;
     }
   }
-  if (registry == value) {
-    // Special case: if the value is an exact match, it is itself a
-    // top-level registry. However, in this case, we want to return 0,
-    // to indicate that it's not allowed to set cookies, etc on the
-    // top-level registry.
-    return 0;
-  }
   if (registry < value || registry >= value_end) {
     // Error cases.
     DCHECK(registry >= value);
@@ -216,9 +208,6 @@ static size_t GetRegistryLengthImpl(
     return 0;
   }
   match_len = value_end - registry;
-  if (match_len >= value_len) {
-    return 0;
-  }
   return match_len;
 }
 
@@ -240,7 +229,7 @@ size_t GetRegistryLength(const char* hostname) {
   DCHECK(*buf_end == 0);
 
   // Normalize the input by converting all characters to lowercase.
-  ToLower(buf, buf_end);
+  ToLowerASCII(buf, buf_end);
   registry_length = GetRegistryLengthImpl(buf, buf_end, '\0', 0);
   free(buf);
   return registry_length;
@@ -264,7 +253,7 @@ size_t GetRegistryLengthAllowUnknownRegistries(const char* hostname) {
   DCHECK(*buf_end == 0);
 
   // Normalize the input by converting all characters to lowercase.
-  ToLower(buf, buf_end);
+  ToLowerASCII(buf, buf_end);
   registry_length = GetRegistryLengthImpl(buf, buf_end, '\0', 1);
   free(buf);
   return registry_length;
index c8d36ec..f2ca16e 100644 (file)
@@ -68,7 +68,7 @@ TEST_F(RegistrySearchTest, FooDotCom) {
   EXPECT_EQ(0, GetRegistryLength("com"));
   EXPECT_EQ(0, GetRegistryLength("bar.com"));
   EXPECT_EQ(0, GetRegistryLength("www.bar.com"));
-  EXPECT_EQ(0, GetRegistryLength("foo.com"));
+  EXPECT_EQ(7, GetRegistryLength("foo.com"));
   EXPECT_EQ(7, GetRegistryLength("a.foo.com"));
   EXPECT_EQ(7, GetRegistryLength("b.foo.com"));
   EXPECT_EQ(7, GetRegistryLength("zzz.foo.com"));
@@ -78,7 +78,7 @@ TEST_F(RegistrySearchTest, FooDotCom) {
 TEST_F(RegistrySearchTest, DotFoo) {
   EXPECT_EQ(0, GetRegistryLength("foo"));
   EXPECT_EQ(0, GetRegistryLength(".foo"));
-  EXPECT_EQ(0, GetRegistryLength("zzz.foo"));
+  EXPECT_EQ(7, GetRegistryLength("zzz.foo"));
 }
 
 TEST_F(RegistrySearchTest, BazDotFoo) {
@@ -88,16 +88,16 @@ TEST_F(RegistrySearchTest, BazDotFoo) {
 }
 
 TEST_F(RegistrySearchTest, BarDotFoo) {
-  EXPECT_EQ(0, GetRegistryLength("bar.foo"));
-  EXPECT_EQ(0, GetRegistryLength(".bar.foo"));
+  EXPECT_EQ(7, GetRegistryLength("bar.foo"));
+  EXPECT_EQ(7, GetRegistryLength(".bar.foo"));
 
   // Our ruleset includes both "bar.foo" and "*.bar.foo". In all
   // cases, "*.bar.foo" should take precendence, since the list format
   // specification says: "If a hostname matches more than one rule in
   // the file, the longest matching rule (the one with the most
   // levels) will be used.".
-  EXPECT_EQ(0, GetRegistryLength("www.bar.foo"));
-  EXPECT_EQ(0, GetRegistryLength("foo.bar.foo"));
+  EXPECT_EQ(11, GetRegistryLength("www.bar.foo"));
+  EXPECT_EQ(11, GetRegistryLength("foo.bar.foo"));
 
   // Tests for children of bar.foo (foo.bar.foo,
   // *.bar.foo). foo.bar.foo is redundant however we want to verify
@@ -107,7 +107,7 @@ TEST_F(RegistrySearchTest, BarDotFoo) {
   EXPECT_EQ(11, GetRegistryLength("www.zzz.bar.foo"));
   EXPECT_EQ(12, GetRegistryLength("www.asdf.bar.foo"));
   EXPECT_EQ(9, GetRegistryLength("z.a.bar.foo"));
-  EXPECT_EQ(0, GetRegistryLength(".a.bar.foo"));
+  EXPECT_EQ(9, GetRegistryLength(".a.bar.foo"));
 }
 
 TEST_F(RegistrySearchTest, StarDotFoo) {
@@ -121,18 +121,18 @@ TEST_F(RegistrySearchTest, StarDotFoo) {
   EXPECT_EQ(7, GetRegistryLength("www.baz.zzz.foo"));
 
   // foo.*.foo:
-  EXPECT_EQ(0, GetRegistryLength("foo.a.foo"));
-  EXPECT_EQ(0, GetRegistryLength(".foo.a.foo"));
-  EXPECT_EQ(0, GetRegistryLength("..foo.a.foo"));
+  EXPECT_EQ(9, GetRegistryLength("foo.a.foo"));
+  EXPECT_EQ(9, GetRegistryLength(".foo.a.foo"));
+  EXPECT_EQ(9, GetRegistryLength("..foo.a.foo"));
   EXPECT_EQ(9, GetRegistryLength("www.foo.a.foo"));
   EXPECT_EQ(9, GetRegistryLength("www.foo.b.foo"));
   EXPECT_EQ(11, GetRegistryLength("www.foo.zzz.foo"));
   EXPECT_EQ(11, GetRegistryLength("www.foo.zzz.foo"));
 
   // *.*.foo:
-  EXPECT_EQ(0, GetRegistryLength("a.a.foo"));
-  EXPECT_EQ(0, GetRegistryLength("b.b.foo"));
-  EXPECT_EQ(0, GetRegistryLength("zzz.zzz.foo"));
+  EXPECT_EQ(7, GetRegistryLength("a.a.foo"));
+  EXPECT_EQ(7, GetRegistryLength("b.b.foo"));
+  EXPECT_EQ(11, GetRegistryLength("zzz.zzz.foo"));
   EXPECT_EQ(7, GetRegistryLength("a.a.a.foo"));
   EXPECT_EQ(7, GetRegistryLength("b.b.b.foo"));
   EXPECT_EQ(11, GetRegistryLength("www.zzz.zzz.foo"));
@@ -144,9 +144,9 @@ TEST_F(RegistrySearchTest, UnknownRegistries) {
   EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries(".foo.bar"));
   EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("..foo.bar"));
   EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("foo..bar"));
-  EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries("bar"));
-  EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries(".bar"));
-  EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries("..bar"));
+  EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("bar"));
+  EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries(".bar"));
+  EXPECT_EQ(3, GetRegistryLengthAllowUnknownRegistries("..bar"));
   EXPECT_EQ(0, GetRegistryLengthAllowUnknownRegistries("a.foo.com.."));
 }
 
index d9c4ece..d72f471 100644 (file)
@@ -28,9 +28,6 @@ static const char kUpperLowerDistance = 'A' - 'a';
 
 static inline int IsWildcardComponent(const char* component) {
   if (component[0] == '*') {
-    // Wildcards should always appear by themselves. It is an error
-    // for the wildcard character to appear adjacent to a non-NULL byte.
-    DCHECK(component[1] == 0);
     return 1;
   }
   return 0;
@@ -38,10 +35,6 @@ static inline int IsWildcardComponent(const char* component) {
 
 static inline int IsExceptionComponent(const char* component) {
   if (component[0] == '!') {
-    // Exceptions should always appear with a string immediately
-    // after. It is an error for the exception character to appear by
-    // itself.
-    DCHECK(component[1] != 0);
     return 1;
   }
   return 0;
@@ -64,7 +57,7 @@ static inline void ReplaceChar(char* value, char old, char newval) {
   }
 }
 
-static inline void ToLower(char* buf, const char* end) {
+static inline void ToLowerASCII(char* buf, const char* end) {
   for (; buf < end; ++buf) {
     char c = *buf;
     if (c >= 'A' && c <= 'Z') {
index 1c8633d..22ea627 100644 (file)
@@ -29,24 +29,14 @@ TEST(StringUtilTest, IsWildcardComponent) {
   ASSERT_TRUE(IsWildcardComponent("*"));
   ASSERT_FALSE(IsWildcardComponent(""));
   ASSERT_FALSE(IsWildcardComponent("com"));
-#ifdef NDEBUG
   ASSERT_TRUE(IsWildcardComponent("*foo"));
-#else
-  ASSERT_DEATH(IsWildcardComponent("*foo"),
-               "CHECK failed: component\\[1\\] == 0");
-#endif
 }
 
 TEST(StringUtilTest, IsExceptionComponent) {
   ASSERT_TRUE(IsExceptionComponent("!foo"));
   ASSERT_FALSE(IsExceptionComponent(""));
   ASSERT_FALSE(IsExceptionComponent("foo"));
-#ifdef NDEBUG
   ASSERT_TRUE(IsExceptionComponent("!"));
-#else
-  ASSERT_DEATH(IsExceptionComponent("!"),
-               "CHECK failed: component\\[1\\] != 0");
-#endif
 }
 
 TEST(StringUtilTest, ReplaceChar) {
@@ -58,9 +48,9 @@ TEST(StringUtilTest, ReplaceChar) {
   free(hostname);
 }
 
-TEST(StringUtilTest, ToLower) {
+TEST(StringUtilTest, ToLowerASCII) {
   char* hostname = strdup(kHostname);
-  ToLower(hostname, hostname + kHostnameLen);
+  ToLowerASCII(hostname, hostname + kHostnameLen);
   ASSERT_STREQ("foo.bar.com", hostname);
   free(hostname);
 }
index bf3ab43..e5e4208 100644 (file)
@@ -28,7 +28,7 @@ static const struct TrieNode kSimpleNodeTable[] = {
   // 2. first_child_offset. Note that leaf table offsets start at 5.
   // 3. num_children
   // 4. is_terminal
-  { 0,  7, 1, 0 },  // com       (1 leaf child at offset 1)
+  { 0,  7, 1, 0 },  // com       (1 leaf child at offset 2)
   { 4,  2, 3, 0 },  // foo       (3 non-leaf children at offset 2)
   { 10, 0, 0, 1 },  // !baz.foo  (0 children)
   { 8,  5, 3, 1 },  // *.foo     (3 leaf children at offset 0)