Bug 1273749 - Address misc issues with nsNSSCertValidity. r=keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Thu, 19 May 2016 17:35:09 -0700
changeset 369012 9c542a9a44534063e9047106b9d7823776a38cce
parent 369011 a3eb07c249c423d3ff2ee258665be51480a404be
child 521424 8d49c8c81b2f57e55dc5db4cfd3270a7555f8119
push id18695
push usercykesiopka.bmo@gmail.com
push dateFri, 20 May 2016 01:41:55 +0000
reviewerskeeler
bugs1273749
milestone49.0a1
Bug 1273749 - Address misc issues with nsNSSCertValidity. r=keeler Prior to the changes here, nsNSSCertValidity had the following issues: - Did not check for NSS shut down. - Provided an irrelevant zero argument constructor. - Did not explicitly delete the unwanted copy constructor and assignment operators. - Misc style issues. - Did not have a dedicated test. MozReview-Commit-ID: JUPtk1OjsNg
security/manager/ssl/nsNSSCertValidity.cpp
security/manager/ssl/nsNSSCertValidity.h
security/manager/ssl/nsNSSCertificate.cpp
security/manager/ssl/tests/unit/head_psm.js
security/manager/ssl/tests/unit/test_nsIX509CertValidity.js
security/manager/ssl/tests/unit/test_sdr.js
security/manager/ssl/tests/unit/xpcshell.ini
--- a/security/manager/ssl/nsNSSCertValidity.cpp
+++ b/security/manager/ssl/nsNSSCertValidity.cpp
@@ -1,54 +1,61 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsNSSCertValidity.h"
+
+#include "cert.h"
+#include "mozilla/Assertions.h"
 #include "nsCOMPtr.h"
-#include "nsIDateTimeFormat.h"
 #include "nsComponentManagerUtils.h"
 #include "nsReadableUtils.h"
-#include "nsNSSShutDown.h"
-#include "cert.h"
 
-/* Implementation file */
 NS_IMPL_ISUPPORTS(nsX509CertValidity, nsIX509CertValidity)
 
-nsX509CertValidity::nsX509CertValidity() : mTimesInitialized(false)
+nsX509CertValidity::nsX509CertValidity(const mozilla::UniqueCERTCertificate& cert)
+  : mTimesInitialized(false)
 {
-  /* member initializers and constructor code */
-}
+  MOZ_ASSERT(cert);
+  if (!cert) {
+    return;
+  }
 
-nsX509CertValidity::nsX509CertValidity(CERTCertificate *cert) : 
-                                           mTimesInitialized(false)
-{
   nsNSSShutDownPreventionLock locker;
-  if (cert) {
-    SECStatus rv = CERT_GetCertTimes(cert, &mNotBefore, &mNotAfter);
-    if (rv == SECSuccess)
-      mTimesInitialized = true;
+  if (isAlreadyShutDown()) {
+    return;
+  }
+
+  if (CERT_GetCertTimes(cert.get(), &mNotBefore, &mNotAfter) == SECSuccess) {
+    mTimesInitialized = true;
   }
 }
 
 nsX509CertValidity::~nsX509CertValidity()
 {
-  /* destructor code */
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return;
+  }
+
+  shutdown(calledFromObject);
 }
 
-NS_IMETHODIMP nsX509CertValidity::GetNotBefore(PRTime *aNotBefore)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotBefore(PRTime* aNotBefore)
 {
   NS_ENSURE_ARG(aNotBefore);
 
-  nsresult rv = NS_ERROR_FAILURE;
-  if (mTimesInitialized) {
-    *aNotBefore = mNotBefore;
-    rv = NS_OK;
+  if (!mTimesInitialized) {
+    return NS_ERROR_FAILURE;
   }
-  return rv;
+
+  *aNotBefore = mNotBefore;
+  return NS_OK;
 }
 
 nsresult
 nsX509CertValidity::FormatTime(const PRTime& aTimeDate,
                                PRTimeParamFn aParamFn,
                                const nsTimeFormatSelector aTimeFormatSelector,
                                nsAString& aFormattedTimeDate)
 {
@@ -62,56 +69,62 @@ nsX509CertValidity::FormatTime(const PRT
 
   PRExplodedTime explodedTime;
   PR_ExplodeTime(const_cast<PRTime&>(aTimeDate), aParamFn, &explodedTime);
   return dateFormatter->FormatPRExplodedTime(nullptr, kDateFormatLong,
 					     aTimeFormatSelector,
 					     &explodedTime, aFormattedTimeDate);
 }
 
-NS_IMETHODIMP nsX509CertValidity::GetNotBeforeLocalTime(nsAString &aNotBeforeLocalTime)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotBeforeLocalTime(nsAString& aNotBeforeLocalTime)
 {
   return FormatTime(mNotBefore, PR_LocalTimeParameters,
                     kTimeFormatSeconds, aNotBeforeLocalTime);
 }
 
-NS_IMETHODIMP nsX509CertValidity::GetNotBeforeLocalDay(nsAString &aNotBeforeLocalDay)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotBeforeLocalDay(nsAString& aNotBeforeLocalDay)
 {
   return FormatTime(mNotBefore, PR_LocalTimeParameters,
                     kTimeFormatNone, aNotBeforeLocalDay);
 }
 
-
-NS_IMETHODIMP nsX509CertValidity::GetNotBeforeGMT(nsAString &aNotBeforeGMT)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotBeforeGMT(nsAString& aNotBeforeGMT)
 {
   return FormatTime(mNotBefore, PR_GMTParameters,
                     kTimeFormatSeconds, aNotBeforeGMT);
 }
 
-NS_IMETHODIMP nsX509CertValidity::GetNotAfter(PRTime *aNotAfter)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotAfter(PRTime* aNotAfter)
 {
   NS_ENSURE_ARG(aNotAfter);
 
-  nsresult rv = NS_ERROR_FAILURE;
-  if (mTimesInitialized) {
-    *aNotAfter = mNotAfter;
-    rv = NS_OK;
+  if (!mTimesInitialized) {
+    return NS_ERROR_FAILURE;
   }
-  return rv;
+
+  *aNotAfter = mNotAfter;
+  return NS_OK;
 }
 
-NS_IMETHODIMP nsX509CertValidity::GetNotAfterLocalTime(nsAString &aNotAfterLocaltime)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotAfterLocalTime(nsAString& aNotAfterLocaltime)
 {
   return FormatTime(mNotAfter, PR_LocalTimeParameters,
                     kTimeFormatSeconds, aNotAfterLocaltime);
 }
 
-NS_IMETHODIMP nsX509CertValidity::GetNotAfterLocalDay(nsAString &aNotAfterLocalDay)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotAfterLocalDay(nsAString& aNotAfterLocalDay)
 {
   return FormatTime(mNotAfter, PR_LocalTimeParameters,
                     kTimeFormatNone, aNotAfterLocalDay);
 }
 
-NS_IMETHODIMP nsX509CertValidity::GetNotAfterGMT(nsAString &aNotAfterGMT)
+NS_IMETHODIMP
+nsX509CertValidity::GetNotAfterGMT(nsAString& aNotAfterGMT)
 {
   return FormatTime(mNotAfter, PR_GMTParameters,
                     kTimeFormatSeconds, aNotAfterGMT);
 }
--- a/security/manager/ssl/nsNSSCertValidity.h
+++ b/security/manager/ssl/nsNSSCertValidity.h
@@ -1,36 +1,42 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-#ifndef _NSX509CERTVALIDITY_H_
-#define _NSX509CERTVALIDITY_H_
+#ifndef nsNSSCertValidity_h
+#define nsNSSCertValidity_h
 
+#include "ScopedNSSTypes.h"
 #include "nsIDateTimeFormat.h"
 #include "nsIX509CertValidity.h"
-
-#include "certt.h"
+#include "nsNSSShutDown.h"
 
 class nsX509CertValidity : public nsIX509CertValidity
+                         , public nsNSSShutDownObject
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIX509CERTVALIDITY
 
-  nsX509CertValidity();
-  explicit nsX509CertValidity(CERTCertificate *cert);
+  explicit nsX509CertValidity(const mozilla::UniqueCERTCertificate& cert);
 
 protected:
   virtual ~nsX509CertValidity();
-  /* additional members */
+
+  // Nothing to release.
+  virtual void virtualDestroyNSSReference() override {}
 
 private:
   nsresult FormatTime(const PRTime& aTime,
                       PRTimeParamFn aParamFn,
                       const nsTimeFormatSelector aTimeFormatSelector,
                       nsAString& aFormattedTimeDate);
 
-  PRTime mNotBefore, mNotAfter;
+  PRTime mNotBefore;
+  PRTime mNotAfter;
   bool mTimesInitialized;
+
+  nsX509CertValidity(const nsX509CertValidity& x) = delete;
+  nsX509CertValidity& operator=(const nsX509CertValidity& x) = delete;
 };
 
-#endif
+#endif // nsNSSCertValidity_h
--- a/security/manager/ssl/nsNSSCertificate.cpp
+++ b/security/manager/ssl/nsNSSCertificate.cpp
@@ -1270,18 +1270,22 @@ nsNSSCertificate::GetCert()
 NS_IMETHODIMP
 nsNSSCertificate::GetValidity(nsIX509CertValidity** aValidity)
 {
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown())
     return NS_ERROR_NOT_AVAILABLE;
 
   NS_ENSURE_ARG(aValidity);
-  RefPtr<nsX509CertValidity> validity = new nsX509CertValidity(mCert.get());
 
+  if (!mCert) {
+    return NS_ERROR_FAILURE;
+  }
+
+  nsCOMPtr<nsIX509CertValidity> validity = new nsX509CertValidity(mCert);
   validity.forget(aValidity);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSCertificate::GetUsagesArray(bool localOnly,
                                  uint32_t* _verified,
                                  uint32_t* _count,
--- a/security/manager/ssl/tests/unit/head_psm.js
+++ b/security/manager/ssl/tests/unit/head_psm.js
@@ -1,16 +1,18 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 "use strict";
 
 const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 
+const { AppConstants } =
+  Cu.import("resource://gre/modules/AppConstants.jsm", {});
 const { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm", {});
 const { FileUtils } = Cu.import("resource://gre/modules/FileUtils.jsm", {});
 const { HttpServer } = Cu.import("resource://testing-common/httpd.js", {});
 const { MockRegistrar } =
   Cu.import("resource://testing-common/MockRegistrar.jsm", {});
 const { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {});
 const { Promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
 const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
new file mode 100644
--- /dev/null
+++ b/security/manager/ssl/tests/unit/test_nsIX509CertValidity.js
@@ -0,0 +1,49 @@
+// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/publicdomain/zero/1.0/
+"use strict";
+
+// This file tests the nsIX509CertValidity implementation.
+
+function fuzzyEqual(attributeName, dateString, expectedTime) {
+  do_print(`${attributeName}: ${dateString}`);
+  let absTimeDiff = Math.abs(expectedTime - Date.parse(dateString));
+  const ONE_DAY_IN_MS = 24 * 60 * 60 * 1000;
+  lessOrEqual(absTimeDiff, ONE_DAY_IN_MS,
+              `Time difference for ${attributeName} should be <= one day`);
+}
+
+function run_test() {
+  // Date.parse("2013-01-01T00:00:00Z")
+  const NOT_BEFORE_IN_MS = 1356998400000;
+  // Date.parse("2014-01-01T00:00:00Z")
+  const NOT_AFTER_IN_MS = 1388534400000;
+  let cert = constructCertFromFile("bad_certs/expired-ee.pem");
+
+  equal(cert.validity.notBefore, NOT_BEFORE_IN_MS * 1000,
+        "Actual and expected notBefore should be equal");
+  equal(cert.validity.notAfter, NOT_AFTER_IN_MS * 1000,
+        "Actual and expected notAfter should be equal");
+
+  // The following tests rely on the implementation of nsIX509CertValidity
+  // providing long formatted dates to work. If this is not true, a returned
+  // short formatted date such as "12/31/12" may be interpreted as something
+  // other than the expected "December 31st, 2012".
+  //
+  // On Android, the implementation of nsIDateTimeFormat currently does not
+  // return a long formatted date even if it is asked to. This, combined with
+  // the reason above is why the following tests are disabled on Android.
+  if (AppConstants.platform != "android") {
+    fuzzyEqual("notBeforeLocalTime", cert.validity.notBeforeLocalTime,
+               NOT_BEFORE_IN_MS);
+    fuzzyEqual("notBeforeLocalDay", cert.validity.notBeforeLocalDay,
+               NOT_BEFORE_IN_MS);
+    fuzzyEqual("notBeforeGMT", cert.validity.notBeforeGMT, NOT_BEFORE_IN_MS);
+
+    fuzzyEqual("notAfterLocalTime", cert.validity.notAfterLocalTime,
+               NOT_AFTER_IN_MS);
+    fuzzyEqual("notAfterLocalDay", cert.validity.notAfterLocalDay,
+               NOT_AFTER_IN_MS);
+    fuzzyEqual("notAfterGMT", cert.validity.notAfterGMT, NOT_AFTER_IN_MS);
+  }
+}
--- a/security/manager/ssl/tests/unit/test_sdr.js
+++ b/security/manager/ssl/tests/unit/test_sdr.js
@@ -1,18 +1,15 @@
 // -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
 // Any copyright is dedicated to the Public Domain.
 // http://creativecommons.org/publicdomain/zero/1.0/
 "use strict";
 
 // Tests various aspects of the nsISecretDecoderRing implementation.
 
-const { AppConstants } =
-  Cu.import("resource://gre/modules/AppConstants.jsm", {});
-
 do_get_profile();
 
 let gSetPasswordShownCount = 0;
 
 // Mock implementation of nsITokenPasswordDialogs.
 const gTokenPasswordDialogs = {
   setPassword: (ctx, tokenName, canceled) => {
     gSetPasswordShownCount++;
--- a/security/manager/ssl/tests/unit/xpcshell.ini
+++ b/security/manager/ssl/tests/unit/xpcshell.ini
@@ -74,16 +74,17 @@ skip-if = toolkit == 'gonk' && debug
 run-sequentially = hardcoded ports
 [test_local_cert.js]
 [test_logoutAndTeardown.js]
 run-sequentially = hardcoded ports
 [test_name_constraints.js]
 [test_nsCertType.js]
 run-sequentially = hardcoded ports
 [test_nsIX509Cert_utf8.js]
+[test_nsIX509CertValidity.js]
 [test_ocsp_caching.js]
 run-sequentially = hardcoded ports
 [test_ocsp_enabled_pref.js]
 run-sequentially = hardcoded ports
 [test_ocsp_fetch_method.js]
 # OCSP requests in this test time out on slow B2G Emulator debug builds.
 # See Bug 1147725.
 skip-if = toolkit == 'gonk' && debug