Bug 1128413, Part 3: Enable more compiler warnings, r=mmc

security/pkix/include/pkix/Input.h

@@ -80,6 +80,9 @@ public:
   {
   }
 
+  // This is intentionally not explicit in order to allow value semantics.
+  Input(const Input&) = default;
+
   // Initialize the input. data must be non-null and len must be less than
   // 65536. Init may not be called more than once.
   Result Init(const uint8_t* data, size_t len)
@@ -271,7 +274,7 @@ public:
 
   Result SkipToEnd(/*out*/ Input& skipped)
   {
-    return Skip(static_cast<size_t>(end - input), skipped);
+    return Skip(static_cast<Input::size_type>(end - input), skipped);
   }
 
   Result EnsureLength(Input::size_type len)
@@ -286,6 +289,8 @@ public:
 
   class Mark final
   {
+  public:
+    Mark(const Mark&) = default; // Intentionally not explicit.
   private:
     friend class Reader;
     Mark(const Reader& input, const uint8_t* mark) : input(input), mark(mark) { }

security/pkix/lib/pkixder.h

@@ -55,7 +55,7 @@ enum Constructed
   CONSTRUCTED = 1 << 5
 };
 
-enum Tag
+enum Tag : uint8_t
 {
   BOOLEAN = UNIVERSAL | 0x01,
   INTEGER = UNIVERSAL | 0x02,

security/pkix/lib/pkixnames.cpp

@@ -1642,13 +1642,14 @@ FinishIPv6Address(/*in/out*/ uint8_t (&address)[16], int numComponents,
   }
 
   // Shift components that occur after the contraction over.
-  int componentsToMove = numComponents - contractionIndex;
+  size_t componentsToMove = static_cast<size_t>(numComponents -
-  memmove(address + (2u * (8 - componentsToMove)),
+                                                contractionIndex);
-          address + (2u * contractionIndex),
+  memmove(address + (2u * static_cast<size_t>(8 - componentsToMove)),
+          address + (2u * static_cast<size_t>(contractionIndex)),
           componentsToMove * 2u);
   // Fill in the contracted area with zeros.
-  memset(address + (2u * contractionIndex), 0u,
+  memset(address + (2u * static_cast<size_t>(contractionIndex)), 0u,
-         (8u - numComponents) * 2u);
+         (8u - static_cast<size_t>(numComponents)) * 2u);
 
   return true;
 }
@@ -1785,7 +1786,6 @@ ParseIPv6Address(Input hostname, /*out*/ uint8_t (&out)[16])
       if (contractionIndex != -1) {
         return false; // multiple contractions are not allowed.
       }
-      uint8_t b;
       if (input.Read(b) != Success || b != ':') {
         assert(false);
         return false;

security/pkix/lib/pkixtime.cpp

@@ -24,8 +24,15 @@
 
 #include "pkix/Time.h"
 #include "pkixutil.h"
+
 #ifdef WIN32
+#ifdef _MSC_VER
+#pragma warning(push, 3)
+#endif
 #include "windows.h"
+#ifdef _MSC_VER
+#pragma warning(pop)
+#endif
 #else
 #include "sys/time.h"
 #endif
@@ -53,7 +60,8 @@ Now()
   //   - http://pubs.opengroup.org/onlinepubs/009695399/functions/gettimeofday.html
   timeval tv;
   (void) gettimeofday(&tv, nullptr);
-  seconds = (DaysBeforeYear(1970) * Time::ONE_DAY_IN_SECONDS) + tv.tv_sec;
+  seconds = (DaysBeforeYear(1970) * Time::ONE_DAY_IN_SECONDS) +
+            static_cast<uint64_t>(tv.tv_sec);
 #endif
 
   return TimeFromElapsedSecondsAD(seconds);

security/pkix/lib/pkixutil.h

@@ -53,7 +53,7 @@ public:
   Result Init();
 
   const Input GetDER() const { return der; }
-  const der::Version GetVersion() const { return version; }
+  der::Version GetVersion() const { return version; }
   const SignedDataWithSignature& GetSignedData() const { return signedData; }
   const Input GetIssuer() const { return issuer; }
   // XXX: "validity" is a horrible name for the structure that holds
@@ -232,24 +232,25 @@ WrappedVerifySignedData(TrustDomain& trustDomain,
 //         MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM
 //       }
 //     }
-#if defined(__clang__) && (__clang_major__ == 3 && __clang_minor__ < 5)
+#if defined(__clang__)
-  // Earlier versions of Clang will warn if not all cases are covered
+// Clang will warn if not all cases are covered (-Wswitch-enum) AND it will
-  // (-Wswitch-enum) AND they always, inappropriately, assume the default case
+// warn if a switch statement that covers every enum label has a default case
-  // is unreachable. This was fixed in
+// (-W-covered-switch-default). Versions prior to 3.5 warned about unreachable
-  // http://llvm.org/klaus/clang/commit/28cd22d7c2d2458575ce9cc19dfe63c6321010ce/
+// code in such default cases (-Wunreachable-code) even when
-# define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM // empty
+// -W-covered-switch-default was disabled, but that changed in Clang 3.5.
-#elif defined(__GNUC__) || defined(__clang__)
+#define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM // empty
-  // GCC and recent versions of clang will warn if not all cases are covered
+#elif defined(__GNUC__)
-  // (-Wswitch-enum). They do not assume that the default case is unreachable.
+// GCC will warn if not all cases are covered (-Wswitch-enum). It does not
-# define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
+// assume that the default case is unreachable.
-         default: assert(false); __builtin_unreachable();
+#define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
+        default: assert(false); __builtin_unreachable();
 #elif defined(_MSC_VER)
-  // MSVC will warn if not all cases are covered (C4061, level 4). It does not
+// MSVC will warn if not all cases are covered (C4061, level 4). It does not
-  // assume that the default case is unreachable.
+// assume that the default case is unreachable.
-# define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
+#define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
-         default: assert(false); __assume(0);
+        default: assert(false); __assume(0);
 #else
-# error Unsupported compiler for MOZILLA_PKIX_UNREACHABLE_DEFAULT.
+#error Unsupported compiler for MOZILLA_PKIX_UNREACHABLE_DEFAULT.
 #endif
 
 } } // namespace mozilla::pkix

security/pkix/moz.build

@@ -25,18 +25,7 @@ TEST_DIRS += [
     'test/lib',
 ]
 
-CXXFLAGS += ['-Wall']
+include('warnings.mozbuild')
-# -Wall with Visual C++ enables too many problematic warnings
-if CONFIG['_MSC_VER']:
-  CXXFLAGS += [
-    '-wd4514', # 'function': unreferenced inline function has been removed
-    '-wd4668', # 'symbol' is not defined as a preprocessor macro...
-    '-wd4710', # 'function': function not inlined
-    '-wd4711', # function 'function' selected for inline expansion
-    '-wd4820', # 'bytes' bytes padding added after construct 'member_name'
-  ]
-
-FAIL_ON_WARNINGS = True
 
 Library('mozillapkix')
 

security/pkix/test/gtest/moz.build

@@ -30,11 +30,24 @@ LOCAL_INCLUDES += [
 
 FINAL_LIBRARY = 'xul-gtest'
 
-include('/ipc/chromium/chromium-config.mozbuild')
+include('../../warnings.mozbuild')
 
-if CONFIG['_MSC_VER']:
+# These warnings are disabled in order to minimize the amount of boilerplate
+# required to implement tests, and/or because they originate in the GTest
+# framework in a way we cannot otherwise work around.
+if CONFIG['CLANG_CXX']:
   CXXFLAGS += [
-    '-wd4275', # non dll-interface class used as base for dll-interface class
+    '-Wno-exit-time-destructors',
+    '-Wno-global-constructors',
+    '-Wno-old-style-cast',
+    '-Wno-used-but-marked-unused',
+  ]
+elif CONFIG['_MSC_VER']:
+  CXXFLAGS += [
+    '-wd4350', # behavior change: 'std::_Wrap_alloc<std::allocator<_Ty>>::...
+    '-wd4275', # non dll-interface class used as base for dll-interface class
+    '-wd4548', # Expression before comma has no effect
+    '-wd4625', # copy constructor could not be generated.
+    '-wd4626', # assugment operator could not be generated.
+    '-wd4640', # construction of local static object is not thread safe.
   ]
-
-FAIL_ON_WARNINGS = True

security/pkix/test/gtest/pkixbuild_tests.cpp

@@ -22,8 +22,20 @@
  * limitations under the License.
  */
 
+#if defined(_MSC_VER) && _MSC_VER < 1900
+// When building with -D_HAS_EXCEPTIONS=0, MSVC's <xtree> header triggers
+// warning C4702: unreachable code.
+// https://connect.microsoft.com/VisualStudio/feedback/details/809962
+#pragma warning(push)
+#pragma warning(disable: 4702)
+#endif
+
 #include <map>
-#include "cert.h"
+
+#if defined(_MSC_VER) && _MSC_VER < 1900
+#pragma warning(pop)
+#endif
+
 #include "pkix/pkix.h"
 #include "pkixgtest.h"
 #include "pkixtestutil.h"
@@ -110,7 +122,7 @@ private:
     return Success;
   }
 
-  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time time)
+  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time)
                     override
   {
     ByteString subjectDER(InputToByteString(encodedIssuerName));
@@ -147,8 +159,7 @@ private:
     return TestVerifySignedData(signedData, subjectPublicKeyInfo);
   }
 
-  Result DigestBuf(Input item, /*out*/ uint8_t *digestBuf, size_t digestBufLen)
+  Result DigestBuf(Input, /*out*/ uint8_t*, size_t) override
-                   override
   {
     ADD_FAILURE();
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
@@ -261,9 +272,8 @@ public:
   }
 
   // The CertPolicyId argument is unused because we don't care about EV.
-  Result GetCertTrust(EndEntityOrCA endEntityOrCA, const CertPolicyId&,
+  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
-                      Input candidateCert, /*out*/ TrustLevel& trustLevel)
+                      /*out*/ TrustLevel& trustLevel) override
-                      override
   {
     Input rootCert;
     Result rv = rootCert.Init(rootDER.data(), rootDER.length());
@@ -278,8 +288,7 @@ public:
     return Success;
   }
 
-  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time time)
+  Result FindIssuer(Input, IssuerChecker& checker, Time) override
-                    override
   {
     // keepGoing is an out parameter from IssuerChecker.Check. It would tell us
     // whether or not to continue attempting other potential issuers. We only
@@ -343,7 +352,7 @@ TEST_F(pkixbuild, NoRevocationCheckingForExpiredCert)
   ByteString certDER(CreateEncodedCertificate(
                        v3, sha256WithRSAEncryption,
                        serialNumber, issuerDER,
-                       oneDayBeforeNow - Time::ONE_DAY_IN_SECONDS,
+                       oneDayBeforeNow - ONE_DAY_IN_SECONDS_AS_TIME_T,
                        oneDayBeforeNow,
                        subjectDER, *reusedKey, nullptr, *reusedKey,
                        sha256WithRSAEncryption));
@@ -389,8 +398,7 @@ public:
     return Success;
   }
 
-  Result VerifySignedData(const SignedDataWithSignature& signedData,
+  Result VerifySignedData(const SignedDataWithSignature&, Input) override
-                          Input subjectPublicKeyInfo) override
   {
     ADD_FAILURE();
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
@@ -463,7 +471,7 @@ public:
     return Success;
   }
 
-  Result FindIssuer(Input subjectCert, IssuerChecker& checker, Time) override
+  Result FindIssuer(Input, IssuerChecker& checker, Time) override
   {
     Input issuerInput;
     EXPECT_EQ(Success, issuerInput.Init(issuer.data(), issuer.length()));

security/pkix/test/gtest/pkixcert_extension_tests.cpp

@@ -63,7 +63,7 @@ CreateCertWithOneExtension(const char* subjectStr, const ByteString& extension)
 class TrustEverythingTrustDomain final : public TrustDomain
 {
 private:
-  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
+  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input,
                       /*out*/ TrustLevel& trustLevel) override
   {
     trustLevel = TrustLevel::TrustAnchor;

security/pkix/test/gtest/pkixcheck_CheckKeyUsage_tests.cpp

@@ -22,7 +22,7 @@
  * limitations under the License.
  */
 
-#include "gtest/gtest.h"
+#include "pkixgtest.h"
 #include "pkix/pkixtypes.h"
 #include "pkixtestutil.h"
 

security/pkix/test/gtest/pkixcheck_CheckValidity_tests.cpp

@@ -22,7 +22,7 @@
  * limitations under the License.
  */
 
-#include "gtest/gtest.h"
+#include "pkixgtest.h"
 #include "pkix/pkixtypes.h"
 #include "pkixtestutil.h"
 

security/pkix/test/gtest/pkixder_input_tests.cpp

@@ -24,7 +24,7 @@
 
 #include <functional>
 #include <vector>
-#include <gtest/gtest.h>
+#include "pkixgtest.h"
 
 #include "pkixder.h"
 
@@ -467,7 +467,7 @@ TEST_F(pkixder_input_tests, MarkAndGetInput)
 }
 
 // Cannot run this test on debug builds because of the NotReached
-#ifndef DEBUG
+#ifdef NDEBUG
 TEST_F(pkixder_input_tests, MarkAndGetInputDifferentInput)
 {
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
@@ -847,7 +847,7 @@ TEST_F(pkixder_input_tests, NestedOf)
                      [&readValues](Reader& r) {
                        return NestedOfHelper(r, readValues);
                      }));
-  ASSERT_EQ((size_t) 3, readValues.size());
+  ASSERT_EQ(3u, readValues.size());
   ASSERT_EQ(0x01, readValues[0]);
   ASSERT_EQ(0x02, readValues[1]);
   ASSERT_EQ(0x03, readValues[2]);
@@ -865,7 +865,7 @@ TEST_F(pkixder_input_tests, NestedOfWithTruncatedData)
                      [&readValues](Reader& r) {
                        return NestedOfHelper(r, readValues);
                      }));
-  ASSERT_EQ((size_t) 0, readValues.size());
+  ASSERT_EQ(0u, readValues.size());
 }
 
 TEST_F(pkixder_input_tests, MatchRestAtEnd)

security/pkix/test/gtest/pkixder_pki_types_tests.cpp

@@ -25,7 +25,7 @@
 #include <functional>
 #include <vector>
 
-#include "gtest/gtest.h"
+#include "pkixgtest.h"
 #include "pkix/pkixtypes.h"
 #include "pkixder.h"
 

security/pkix/test/gtest/pkixder_universal_types_tests.cpp

@@ -24,7 +24,7 @@
 
 #include <limits>
 #include <vector>
-#include <gtest/gtest.h>
+#include "pkixgtest.h"
 
 #include "pkixder.h"
 #include "pkixtestutil.h"
@@ -287,8 +287,9 @@ TEST_F(pkixder_universal_types_tests, EnumeratedInvalidZeroLength)
 // other encodings is actually encouraged.
 
 // e.g. TWO_CHARS(53) => '5', '3'
-#define TWO_CHARS(t) static_cast<uint8_t>('0' + ((t) / 10u)), \
+#define TWO_CHARS(t) \
-                     static_cast<uint8_t>('0' + ((t) % 10u))
+  static_cast<uint8_t>('0' + (static_cast<uint8_t>(t) / 10u)), \
+  static_cast<uint8_t>('0' + (static_cast<uint8_t>(t) % 10u))
 
 // Calls TimeChoice on the UTCTime variant of the given generalized time.
 template <uint16_t LENGTH>
@@ -555,7 +556,7 @@ static const uint8_t DAYS_IN_MONTH[] = {
 
 TEST_F(pkixder_universal_types_tests, TimeMonthDaysValidRange)
 {
-  for (uint8_t month = 1; month <= 12; ++month) {
+  for (uint16_t month = 1; month <= 12; ++month) {
     for (uint8_t day = 1; day <= DAYS_IN_MONTH[month]; ++day) {
       const uint8_t DER[] = {
         0x18,                           // Generalized Time
@@ -604,7 +605,7 @@ TEST_F(pkixder_universal_types_tests, TimeDayInvalid0)
 
 TEST_F(pkixder_universal_types_tests, TimeMonthDayInvalidPastEndOfMonth)
 {
-  for (uint8_t month = 1; month <= 12; ++month) {
+  for (int16_t month = 1; month <= 12; ++month) {
     const uint8_t DER[] = {
       0x18,                           // Generalized Time
       15,                             // Length = 15
@@ -905,7 +906,7 @@ TEST_F(pkixder_universal_types_tests, Integer_0_127)
     Input input(DER);
     Reader reader(input);
 
-    uint8_t value = i + 1; // initialize with a value that is NOT i.
+    uint8_t value = i + 1u; // initialize with a value that is NOT i.
     ASSERT_EQ(Success, Integer(reader, value));
     ASSERT_EQ(i, value);
   }

security/pkix/test/gtest/pkixgtest.cpp

@@ -30,10 +30,15 @@
 
 namespace mozilla { namespace pkix { namespace test {
 
+const std::time_t ONE_DAY_IN_SECONDS_AS_TIME_T =
+  static_cast<std::time_t>(Time::ONE_DAY_IN_SECONDS);
+
 // This assumes that time/time_t are POSIX-compliant in that time() returns
 // the number of seconds since the Unix epoch.
 const std::time_t now(time(nullptr));
-const std::time_t oneDayBeforeNow(time(nullptr) - Time::ONE_DAY_IN_SECONDS);
+const std::time_t oneDayBeforeNow(time(nullptr) -
-const std::time_t oneDayAfterNow(time(nullptr) + Time::ONE_DAY_IN_SECONDS);
+                                  ONE_DAY_IN_SECONDS_AS_TIME_T);
+const std::time_t oneDayAfterNow(time(nullptr) +
+                                 ONE_DAY_IN_SECONDS_AS_TIME_T);
 
 } } } // namespace mozilla::pkix::test

security/pkix/test/gtest/pkixgtest.h

@@ -26,7 +26,36 @@
 
 #include <ostream>
 
+#if defined(__clang__)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated"
+#pragma clang diagnostic ignored "-Wmissing-noreturn"
+#pragma clang diagnostic ignored "-Wshift-sign-overflow"
+#pragma clang diagnostic ignored "-Wsign-conversion"
+#pragma clang diagnostic ignored "-Wundef"
+#elif defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wextra"
+#elif defined(_MSC_VER)
+#pragma warning(push, 3)
+// C4224: Nonstandard extension used: formal parameter 'X' was previously
+//        defined as a type.
+#pragma warning(disable: 4224)
+// C4826: Conversion from 'type1 ' to 'type_2' is sign - extended. This may
+//        cause unexpected runtime behavior.
+#pragma warning(disable: 4826)
+#endif
+
 #include "gtest/gtest.h"
+
+#if defined(__clang__)
+#pragma clang diagnostic pop
+#elif defined(__GNUC__)
+#pragma GCC diagnostic pop
+#elif defined(_MSC_VER)
+#pragma warning(pop)
+#endif
+
 #include "pkix/Result.h"
 
 // PrintTo must be in the same namespace as the type we're overloading it for.
@@ -47,6 +76,8 @@ PrintTo(const Result& result, ::std::ostream* os)
 
 namespace mozilla { namespace pkix { namespace test {
 
+extern const std::time_t ONE_DAY_IN_SECONDS_AS_TIME_T;
+
 extern const std::time_t now;
 extern const std::time_t oneDayBeforeNow;
 extern const std::time_t oneDayAfterNow;

security/pkix/test/gtest/pkixnames_tests.cpp

@@ -72,7 +72,8 @@ struct PresentedMatchesReference
   { \
     ByteString(reinterpret_cast<const uint8_t*>(a), sizeof(a) - 1), \
     ByteString(reinterpret_cast<const uint8_t*>(b), sizeof(b) - 1), \
-    Result::ERROR_BAD_DER \
+    Result::ERROR_BAD_DER, \
+    false \
   }
 
 static const PresentedMatchesReference DNSID_MATCH_PARAMS[] =

security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp

@@ -22,7 +22,7 @@
  * limitations under the License.
  */
 
-#include "gtest/gtest.h"
+#include "pkixgtest.h"
 #include "pkix/pkix.h"
 #include "pkixder.h"
 #include "pkixtestutil.h"
@@ -71,7 +71,7 @@ private:
     return TestDigestBuf(item, digestBuf, digestBufLen);
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) override
+  Result CheckPublicKey(Input) override
   {
     ADD_FAILURE();
     return Result::FATAL_ERROR_LIBRARY_FAILURE;

security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp

@@ -53,7 +53,7 @@ public:
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  Result CheckRevocation(EndEntityOrCA endEntityOrCA, const CertID&, Time time,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          final override
   {
@@ -119,7 +119,8 @@ public:
       abort();
     }
 
-    serialNumberDER = CreateEncodedSerialNumber(++rootIssuedCount);
+    serialNumberDER =
+      CreateEncodedSerialNumber(static_cast<long>(++rootIssuedCount));
     if (ENCODING_FAILED(serialNumberDER)) {
       abort();
     }
@@ -143,7 +144,7 @@ public:
   }
 
   static ScopedTestKeyPair rootKeyPair;
-  static long rootIssuedCount;
+  static uint32_t rootIssuedCount;
   OCSPTestTrustDomain trustDomain;
 
   // endEntityCertID references rootKeyPair, rootNameDER, and serialNumberDER.
@@ -154,7 +155,7 @@ public:
 };
 
 /*static*/ ScopedTestKeyPair pkixocsp_VerifyEncodedResponse::rootKeyPair;
-/*static*/ long pkixocsp_VerifyEncodedResponse::rootIssuedCount = 0;
+/*static*/ uint32_t pkixocsp_VerifyEncodedResponse::rootIssuedCount = 0;
 
 ///////////////////////////////////////////////////////////////////////////////
 // responseStatus
@@ -258,7 +259,7 @@ public:
     context.signatureAlgorithm = signatureAlgorithm;
     context.certs = certs;
 
-    context.certStatus = certStatus;
+    context.certStatus = static_cast<uint8_t>(certStatus);
     context.thisUpdate = thisUpdate;
     context.nextUpdate = nextUpdate ? *nextUpdate : 0;
     context.includeNextUpdate = nextUpdate != nullptr;
@@ -408,7 +409,8 @@ TEST_F(pkixocsp_VerifyEncodedResponse_successful, check_validThrough)
     ASSERT_FALSE(expired);
     // The response was created to be valid until one day after now, so the
     // value we got for validThrough should be after that.
-    Time oneDayAfterNowAsPKIXTime(TimeFromEpochInSeconds(oneDayAfterNow));
+    Time oneDayAfterNowAsPKIXTime(
+          TimeFromEpochInSeconds(static_cast<uint64_t>(oneDayAfterNow)));
     ASSERT_TRUE(validThrough > oneDayAfterNowAsPKIXTime);
   }
   {
@@ -518,7 +520,8 @@ protected:
                                 /*optional*/ const ByteString* extensions,
                                              const TestKeyPair& signerKeyPair)
   {
-    ByteString serialNumberDER(CreateEncodedSerialNumber(serialNumber));
+    ByteString serialNumberDER(CreateEncodedSerialNumber(
+                                 static_cast<long>(serialNumber)));
     if (ENCODING_FAILED(serialNumberDER)) {
       return ByteString();
     }
@@ -634,8 +637,8 @@ TEST_F(pkixocsp_VerifyEncodedResponse_DelegatedResponder, good_expired)
   ScopedTestKeyPair signerKeyPair(GenerateKeyPair());
   ByteString signerDER(CreateEncodedCertificate(
                           ++rootIssuedCount, sha256WithRSAEncryption, rootName,
-                          now - (10 * Time::ONE_DAY_IN_SECONDS),
+                          now - (10 * ONE_DAY_IN_SECONDS_AS_TIME_T),
-                          now - (2 * Time::ONE_DAY_IN_SECONDS),
+                          now - (2 * ONE_DAY_IN_SECONDS_AS_TIME_T),
                           signerName, *signerKeyPair, extensions,
                           *rootKeyPair));
   ASSERT_FALSE(ENCODING_FAILED(signerDER));
@@ -670,8 +673,8 @@ TEST_F(pkixocsp_VerifyEncodedResponse_DelegatedResponder, good_future)
   ByteString signerDER(CreateEncodedCertificate(
                          ++rootIssuedCount, sha256WithRSAEncryption,
                          rootName,
-                         now + (2 * Time::ONE_DAY_IN_SECONDS),
+                         now + (2 * ONE_DAY_IN_SECONDS_AS_TIME_T),
-                         now + (10 * Time::ONE_DAY_IN_SECONDS),
+                         now + (10 * ONE_DAY_IN_SECONDS_AS_TIME_T),
                          signerName, *signerKeyPair, extensions,
                          *rootKeyPair));
   ASSERT_FALSE(ENCODING_FAILED(signerDER));
@@ -1017,13 +1020,14 @@ TEST_F(pkixocsp_VerifyEncodedResponse_GetCertTrust, ActivelyDistrusted)
 {
   ASSERT_TRUE(trustDomain.SetCertTrust(signerCertDER,
                                        TrustLevel::ActivelyDistrusted));
-  Input response;
+  Input responseInput;
   ASSERT_EQ(Success,
-            response.Init(responseString.data(), responseString.length()));
+            responseInput.Init(responseString.data(),
+                               responseString.length()));
   bool expired;
   ASSERT_EQ(Result::ERROR_OCSP_INVALID_SIGNING_CERT,
             VerifyEncodedOCSPResponse(trustDomain, *endEntityCertID, Now(),
                                       END_ENTITY_MAX_LIFETIME_IN_DAYS,
-                                      response, expired));
+                                      responseInput, expired));
   ASSERT_FALSE(expired);
 }

security/pkix/test/lib/pkixtestnss.cpp

@@ -115,7 +115,8 @@ public:
     }
 
     SECItem signatureItem;
-    if (SEC_SignData(&signatureItem, tbs.data(), tbs.length(),
+    if (SEC_SignData(&signatureItem, tbs.data(),
+                     static_cast<int>(tbs.length()),
                      privateKey.get(), signatureAlgorithmOidTag)
           != SECSuccess) {
       return MapPRErrorCodeToResult(PR_GetError());

security/pkix/test/lib/pkixtestutil.cpp

@@ -127,10 +127,10 @@ TLV(uint8_t tag, const ByteString& value)
   result.push_back(tag);
 
   if (value.length() < 128) {
-    result.push_back(value.length());
+    result.push_back(static_cast<uint8_t>(value.length()));
   } else if (value.length() < 256) {
     result.push_back(0x81u);
-    result.push_back(value.length());
+    result.push_back(static_cast<uint8_t>(value.length()));
   } else if (value.length() < 65536) {
     result.push_back(0x82u);
     result.push_back(static_cast<uint8_t>(value.length() / 256));
@@ -158,7 +158,7 @@ OCSPResponseContext::OCSPResponseContext(const CertID& certID, time_t time)
   , certStatus(good)
   , revocationTime(0)
   , thisUpdate(time)
-  , nextUpdate(time + Time::ONE_DAY_IN_SECONDS)
+  , nextUpdate(time + static_cast<time_t>(Time::ONE_DAY_IN_SECONDS))
   , includeNextUpdate(true)
 {
 }
@@ -202,7 +202,7 @@ ByteString
 Boolean(bool value)
 {
   ByteString encodedValue;
-  encodedValue.push_back(value ? 0xff : 0x00);
+  encodedValue.push_back(value ? 0xffu : 0x00u);
   return TLV(der::BOOLEAN, encodedValue);
 }
 
@@ -266,22 +266,22 @@ TimeToEncodedTime(time_t time, TimeEncoding encoding)
   ByteString value;
 
   if (encoding == GeneralizedTime) {
-    value.push_back('0' + (year / 1000));
+    value.push_back(static_cast<uint8_t>('0' + (year / 1000)));
-    value.push_back('0' + ((year % 1000) / 100));
+    value.push_back(static_cast<uint8_t>('0' + ((year % 1000) / 100)));
   }
 
-  value.push_back('0' + ((year % 100) / 10));
+  value.push_back(static_cast<uint8_t>('0' + ((year % 100) / 10)));
-  value.push_back('0' + (year % 10));
+  value.push_back(static_cast<uint8_t>('0' + (year % 10)));
-  value.push_back('0' + ((exploded.tm_mon + 1) / 10));
+  value.push_back(static_cast<uint8_t>('0' + ((exploded.tm_mon + 1) / 10)));
-  value.push_back('0' + ((exploded.tm_mon + 1) % 10));
+  value.push_back(static_cast<uint8_t>('0' + ((exploded.tm_mon + 1) % 10)));
-  value.push_back('0' + (exploded.tm_mday / 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_mday / 10)));
-  value.push_back('0' + (exploded.tm_mday % 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_mday % 10)));
-  value.push_back('0' + (exploded.tm_hour / 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_hour / 10)));
-  value.push_back('0' + (exploded.tm_hour % 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_hour % 10)));
-  value.push_back('0' + (exploded.tm_min / 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_min / 10)));
-  value.push_back('0' + (exploded.tm_min % 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_min % 10)));
-  value.push_back('0' + (exploded.tm_sec / 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_sec / 10)));
-  value.push_back('0' + (exploded.tm_sec % 10));
+  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_sec % 10)));
   value.push_back('Z');
 
   return TLV(encoding == GeneralizedTime ? der::GENERALIZED_TIME : der::UTCTime,
@@ -315,18 +315,15 @@ TimeToTimeChoice(time_t time)
 }
 
 Time
-YMDHMS(int16_t year, int16_t month, int16_t day,
+YMDHMS(uint16_t year, uint16_t month, uint16_t day,
-       int16_t hour, int16_t minutes, int16_t seconds)
+       uint16_t hour, uint16_t minutes, uint16_t seconds)
 {
   assert(year <= 9999);
   assert(month >= 1);
   assert(month <= 12);
   assert(day >= 1);
-  assert(hour >= 0);
   assert(hour < 24);
-  assert(minutes >= 0);
   assert(minutes < 60);
-  assert(seconds >= 0);
   assert(seconds < 60);
 
   uint64_t days = DaysBeforeYear(year);
@@ -439,6 +436,31 @@ EmptyExtension(Input extnID, Critical critical)
   return TLV(der::SEQUENCE, encoded);
 }
 
+std::string
+GetEnv(const char* name)
+{
+  std::string result;
+
+#ifndef _MSC_VER
+  // XXX: Not thread safe.
+  const char* value = getenv(name);
+  if (value) {
+    result = value;
+  }
+#else
+  char* value = nullptr;
+  size_t valueLength = 0;
+  if (_dupenv_s(&value, &valueLength, name) != 0) {
+    abort();
+  }
+  if (value) {
+    result = value;
+    free(value);
+  }
+#endif
+  return result;
+}
+
 void
 MaybeLogOutput(const ByteString& result, const char* suffix)
 {
@@ -447,8 +469,8 @@ MaybeLogOutput(const ByteString& result, const char* suffix)
   // This allows us to more easily debug the generated output, by creating a
   // file in the directory given by MOZILLA_PKIX_TEST_LOG_DIR for each
   // NOT THREAD-SAFE!!!
-  const char* logPath = getenv("MOZILLA_PKIX_TEST_LOG_DIR");
+  std::string logPath(GetEnv("MOZILLA_PKIX_TEST_LOG_DIR"));
-  if (logPath) {
+  if (!logPath.empty()) {
     static int counter = 0;
 
     std::ostringstream counterStream;
@@ -829,7 +851,7 @@ BasicOCSPResponse(OCSPResponseContext& context)
 //   value            OCTET STRING
 // }
 static ByteString
-OCSPExtension(OCSPResponseContext& context, OCSPResponseExtension& extension)
+OCSPExtension(OCSPResponseExtension& extension)
 {
   ByteString encoded;
   encoded.append(extension.id);
@@ -850,7 +872,7 @@ Extensions(OCSPResponseContext& context)
   ByteString value;
   for (OCSPResponseExtension* extension = context.extensions;
        extension; extension = extension->next) {
-    ByteString extensionEncoded(OCSPExtension(context, *extension));
+    ByteString extensionEncoded(OCSPExtension(*extension));
     if (ENCODING_FAILED(extensionEncoded)) {
       return ByteString();
     }
@@ -915,8 +937,11 @@ ResponderID(OCSPResponseContext& context)
     responderIDType = 2; // byKey
   }
 
-  return TLV(der::CONSTRUCTED | der::CONTEXT_SPECIFIC | responderIDType,
+  // XXX: MSVC 2015 wrongly warns about signed/unsigned conversion without the
-             contents);
+  // static_cast.
+  uint8_t tag = static_cast<uint8_t>(der::CONSTRUCTED | der::CONTEXT_SPECIFIC |
+                                     responderIDType);
+  return TLV(tag, contents);
 }
 
 // KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key
@@ -1047,7 +1072,10 @@ CertStatus(OCSPResponseContext& context)
     case 0:
     case 2:
     {
-      return TLV(der::CONTEXT_SPECIFIC | context.certStatus, ByteString());
+      // XXX: MSVC 2015 wrongly warns about signed/unsigned conversion without
+      // the static cast.
+      return TLV(static_cast<uint8_t>(der::CONTEXT_SPECIFIC |
+                                      context.certStatus), ByteString());
     }
     case 1:
     {

security/pkix/test/lib/pkixtestutil.h

@@ -93,12 +93,8 @@ const ByteString md2WithRSAEncryption(alg_md2WithRSAEncryption,
   MOZILLA_PKIX_ARRAY_LENGTH(alg_md2WithRSAEncryption));
 
 // e.g. YMDHMS(2016, 12, 31, 1, 23, 45) => 2016-12-31:01:23:45 (GMT)
-mozilla::pkix::Time YMDHMS(int16_t year, int16_t month, int16_t day,
+mozilla::pkix::Time YMDHMS(uint16_t year, uint16_t month, uint16_t day,
-                           int16_t hour, int16_t minutes, int16_t seconds);
+                           uint16_t hour, uint16_t minutes, uint16_t seconds);
-
-// e.g. YMDHMS(2016, 12, 31, 1, 23, 45) => 2016-12-31:01:23:45 (GMT)
-mozilla::pkix::Time YMDHMS(int16_t year, int16_t month, int16_t day,
-                           int16_t hour, int16_t minutes, int16_t seconds);
 
 ByteString TLV(uint8_t tag, const ByteString& value);
 ByteString Boolean(bool value);
@@ -168,7 +164,7 @@ template <size_t L>
 inline ByteString
 RFC822Name(const char (&bytes)[L])
 {
-  return RFC822Name(ByteString(reinterpret_cast<const uint8_t (&)[L]>(bytes),
+  return RFC822Name(ByteString(reinterpret_cast<const uint8_t*>(&bytes),
                                L - 1));
 }
 
@@ -183,7 +179,7 @@ template <size_t L>
 inline ByteString
 DNSName(const char (&bytes)[L])
 {
-  return DNSName(ByteString(reinterpret_cast<const uint8_t (&)[L]>(bytes),
+  return DNSName(ByteString(reinterpret_cast<const uint8_t*>(&bytes),
                             L - 1));
 }
 

security/pkix/warnings.mozbuild

@@ -0,0 +1,43 @@
+FAIL_ON_WARNINGS = True
+
+if CONFIG['CLANG_CXX']:
+  CXXFLAGS += [
+    '-Weverything',
+
+    '-Wno-c++98-compat',
+    '-Wno-c++98-compat-pedantic',
+    '-Wno-missing-prototypes',
+    '-Wno-missing-variable-declarations',
+    '-Wno-padded',
+    '-Wno-reserved-id-macro', # XXX: Will be removed in bug 1128413, Part 4.
+    '-Wno-shadow', # XXX: Clang's rules are too strict for constructors.
+    '-Wno-undef', # XXX: Will be removed in bug 1128413, Part 4.
+    '-Wno-weak-vtables', # We rely on the linker to merge the duplicate vtables.
+  ]
+elif CONFIG['_MSC_VER']:
+  CXXFLAGS += [
+    '-sdl', # Enable additional security checks based on Microsoft's SDL.
+
+    '-Wall',
+
+    '-wd4514', # 'function': unreferenced inline function has been removed
+    '-wd4668', # warning C4668: 'X' is not defined as a preprocessor macro,
+               # replacing with '0' for '#if/#elif'.
+    '-wd4710', # 'function': function not inlined
+    '-wd4711', # function 'function' selected for inline expansion
+    '-wd4800', # forcing value to bool 'true' or 'false'
+    '-wd4820', # 'bytes' bytes padding added after construct 'member_name'
+
+    # XXX: We cannot use /Za (Disable Microsoft Extensions) because windows.h
+    # won't copmile with it.
+    '-Zc:forScope', # Standard C++ rules for variable scope in for loops.
+    '-Zc:inline', # Standard C++ rules requiring definition inline functions.
+    '-Zc:rvalueCast', # Standard C++ rules for result of cast being an rvalue.
+    '-Zc:strictStrings', # Standard C++ rule that string literals are const.
+  ]
+else:
+  CXXFLAGS += [
+    '-Wall',
+    '-Wextra',
+    '-pedantic-errors',
+  ]