Bug 1361197 - Don't skip first RDN in CERT_CompareName() r=franziskus
authorTim Taubert <ttaubert@mozilla.com>
Tue, 06 Jun 2017 14:12:47 +0200
changeset 13410 c7c9b58789a19244f3d995b55c37ceec4b095340
parent 13407 92f629d628ca2c50a1d092c5693146cd85a08680
child 13411 e45bc5e967aa6a4dfcf9c52b1217cda5076ecc71
push id2230
push userttaubert@mozilla.com
push dateTue, 06 Jun 2017 12:15:32 +0000
reviewersfranziskus
bugs1361197
Bug 1361197 - Don't skip first RDN in CERT_CompareName() r=franziskus Differential Revision: https://nss-review.dev.mozaws.net/D301
gtests/certdb_gtest/alg1485_unittest.cc
lib/certdb/secname.c
--- a/gtests/certdb_gtest/alg1485_unittest.cc
+++ b/gtests/certdb_gtest/alg1485_unittest.cc
@@ -13,18 +13,30 @@
 
 namespace nss_test {
 
 typedef struct AVATestValuesStr {
   std::string avaString;
   bool expectedResult;
 } AVATestValues;
 
-class Alg1485Test : public ::testing::Test,
-                    public ::testing::WithParamInterface<AVATestValues> {};
+typedef struct AVACompareValuesStr {
+  std::string avaString1;
+  std::string avaString2;
+  SECComparison expectedResult;
+} AVACompareValues;
+
+class Alg1485Test : public ::testing::Test {};
+
+class Alg1485ParseTest : public Alg1485Test,
+                         public ::testing::WithParamInterface<AVATestValues> {};
+
+class Alg1485CompareTest
+    : public Alg1485Test,
+      public ::testing::WithParamInterface<AVACompareValues> {};
 
 static const AVATestValues kAVATestStrings[] = {
     {"CN=Marshall T. Rose, O=Dover Beach Consulting, L=Santa Clara, "
      "ST=California, C=US",
      true},
     {"C=HU,L=Budapest,O=Organization,CN=Example - Qualified Citizen "
      "CA,2.5.4.97=VATHU-10",
      true},
@@ -40,18 +52,41 @@ static const AVATestValues kAVATestStrin
     {"YO=LO", false},             // Unknown Tag, 'YO'
     {"CN=Tester,ZZ=Top", false},  // Unknown tag, 'ZZ'
     // These tests are disabled pending Bug 1363416
     // { "01.02.03=Nope", false }, // Numbers not in minimal form
     // { "000001.0000000001=👌", false },
     // { "CN=Somebody,L=Set,O=Up,C=US,01=The,02=Bomb", false },
 };
 
-TEST_P(Alg1485Test, TryParsingAVAStrings) {
+static const AVACompareValues kAVACompareStrings[] = {
+    {"CN=Max, O=Mozilla, ST=Berlin", "CN=Max, O=Mozilla, ST=Berlin, C=DE",
+     SECLessThan},
+    {"CN=Max, O=Mozilla, ST=Berlin, C=DE", "CN=Max, O=Mozilla, ST=Berlin",
+     SECGreaterThan},
+    {"CN=Max, O=Mozilla, ST=Berlin, C=DE", "CN=Max, O=Mozilla, ST=Berlin, C=DE",
+     SECEqual},
+    {"CN=Max1, O=Mozilla, ST=Berlin, C=DE",
+     "CN=Max2, O=Mozilla, ST=Berlin, C=DE", SECLessThan},
+    {"CN=Max, O=Mozilla, ST=Berlin, C=DE", "CN=Max, O=Mozilla, ST=Berlin, C=US",
+     SECLessThan},
+};
+
+TEST_P(Alg1485ParseTest, TryParsingAVAStrings) {
   const AVATestValues& param(GetParam());
 
   ScopedCERTName certName(CERT_AsciiToName(param.avaString.c_str()));
   ASSERT_EQ(certName != nullptr, param.expectedResult);
 }
 
-INSTANTIATE_TEST_CASE_P(ParseAVAStrings, Alg1485Test,
+TEST_P(Alg1485CompareTest, CompareAVAStrings) {
+  const AVACompareValues& param(GetParam());
+  ScopedCERTName a(CERT_AsciiToName(param.avaString1.c_str()));
+  ScopedCERTName b(CERT_AsciiToName(param.avaString2.c_str()));
+  ASSERT_TRUE(a && b);
+  EXPECT_EQ(param.expectedResult, CERT_CompareName(a.get(), b.get()));
+}
+
+INSTANTIATE_TEST_CASE_P(ParseAVAStrings, Alg1485ParseTest,
                         ::testing::ValuesIn(kAVATestStrings));
+INSTANTIATE_TEST_CASE_P(CompareAVAStrings, Alg1485CompareTest,
+                        ::testing::ValuesIn(kAVACompareStrings));
 }
--- a/lib/certdb/secname.c
+++ b/lib/certdb/secname.c
@@ -563,18 +563,18 @@ CERT_CompareRDN(const CERTRDN *a, const 
             return SECGreaterThan;
     }
     return rv;
 }
 
 SECComparison
 CERT_CompareName(const CERTName *a, const CERTName *b)
 {
-    CERTRDN **ardns, *ardn;
-    CERTRDN **brdns, *brdn;
+    CERTRDN **ardns;
+    CERTRDN **brdns;
     int ac, bc;
     SECComparison rv = SECEqual;
 
     ardns = a->rdns;
     brdns = b->rdns;
 
     /*
     ** Make sure array of rdn's are the same length. If not, then we are
@@ -582,28 +582,18 @@ CERT_CompareName(const CERTName *a, cons
     */
     ac = CountArray((void **)ardns);
     bc = CountArray((void **)brdns);
     if (ac < bc)
         return SECLessThan;
     if (ac > bc)
         return SECGreaterThan;
 
-    for (;;) {
-        if (!ardns++ || !brdns++) {
-            break;
-        }
-        ardn = *ardns;
-        brdn = *brdns;
-        if (!ardn) {
-            break;
-        }
-        rv = CERT_CompareRDN(ardn, brdn);
-        if (rv)
-            return rv;
+    while (rv == SECEqual && *ardns) {
+        rv = CERT_CompareRDN(*ardns++, *brdns++);
     }
     return rv;
 }
 
 /* Moved from certhtml.c */
 SECItem *
 CERT_DecodeAVAValue(const SECItem *derAVAValue)
 {