Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/. r?keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 04 Apr 2017 16:56:26 +0800
changeset 555419 87f7bfe454e30bd86a82d63de5fc6fa5330b7eff
parent 555418 3b9eadd61e260fd307b7fcacc683e9536f70e62e
child 622605 6bd8a642afe3c9fd9790bbf04015a5a4f08e017c
push id52236
push usercykesiopka.bmo@gmail.com
push dateTue, 04 Apr 2017 09:12:57 +0000
reviewerskeeler
bugs1346315
milestone55.0a1
Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/. r?keeler -Wextra implies -Wmissing-field-initializers, but since the latter warning seems to warn about mostly uninteresting instances (XPCOM module definitions etc), we disable it for now. (Note that -Wall is already enabled by default for all directories for gcc and clang.) MozReview-Commit-ID: 8RdF51sLPC8
security/apps/moz.build
security/manager/pki/moz.build
security/manager/ssl/SSLServerCertVerification.cpp
security/manager/ssl/moz.build
--- a/security/apps/moz.build
+++ b/security/apps/moz.build
@@ -19,16 +19,26 @@ LOCAL_INCLUDES += [
     '/security/manager/ssl',
     '/security/pkix/include',
 ]
 
 DEFINES['NSS_ENABLE_ECC'] = 'True'
 for var in ('DLL_PREFIX', 'DLL_SUFFIX'):
     DEFINES[var] = '"%s"' % CONFIG[var]
 
+if CONFIG['GNU_CXX']:
+    CXXFLAGS += [
+        '-Wextra',
+    ]
+
+    # Gecko headers aren't warning-free enough for us to enable these warnings.
+    CXXFLAGS += [
+        '-Wno-unused-parameter',
+    ]
+
 test_ssl_path = '/security/manager/ssl/tests/unit'
 
 headers_arrays_certs = [
     ('xpcshell.inc', 'xpcshellRoot', test_ssl_path + '/test_signed_apps/trusted_ca1.der'),
     ('addons-public.inc', 'addonsPublicRoot', 'addons-public.crt'),
     ('addons-stage.inc', 'addonsStageRoot', 'addons-stage.crt'),
     ('privileged-package-root.inc', 'privilegedPackageRoot', 'privileged-package-root.der'),
 ]
--- a/security/manager/pki/moz.build
+++ b/security/manager/pki/moz.build
@@ -19,8 +19,20 @@ UNIFIED_SOURCES += [
     'nsPKIModule.cpp',
 ]
 
 LOCAL_INCLUDES += [
     '!/dist/public/nss',
 ]
 
 FINAL_LIBRARY = 'xul'
+
+if CONFIG['GNU_CXX']:
+    CXXFLAGS += [
+        '-Wextra',
+        # -Wextra enables this warning, but it's too noisy to be useful.
+        '-Wno-missing-field-initializers',
+    ]
+
+    # Gecko headers aren't warning-free enough for us to enable these warnings.
+    CXXFLAGS += [
+        '-Wno-unused-parameter',
+    ]
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -1521,16 +1521,21 @@ SSLServerCertVerificationJob::Dispatch(
 {
   // Runs on the socket transport thread
   if (!certVerifier || !infoObject || !serverCert) {
     NS_ERROR("Invalid parameters for SSL server cert validation");
     PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
     return SECFailure;
   }
 
+  if (!gCertVerificationThreadPool) {
+    PR_SetError(PR_INVALID_STATE_ERROR, 0);
+    return SECFailure;
+  }
+
   // Copy the certificate list so the runnable can take ownership of it in the
   // constructor.
   // We can safely skip checking if NSS has already shut down here since we're
   // in the middle of verifying a certificate.
   nsNSSShutDownPreventionLock lock;
   UniqueCERTCertList peerCertChainCopy =
     nsNSSCertList::DupCertList(peerCertChain, lock);
   if (!peerCertChainCopy) {
@@ -1539,38 +1544,32 @@ SSLServerCertVerificationJob::Dispatch(
   }
 
   RefPtr<SSLServerCertVerificationJob> job(
     new SSLServerCertVerificationJob(certVerifier, fdForLogging, infoObject,
                                      serverCert, Move(peerCertChainCopy),
                                      stapledOCSPResponse, sctsFromTLSExtension,
                                      providerFlags, time, prtime));
 
-  nsresult nrv;
-  if (!gCertVerificationThreadPool) {
-    nrv = NS_ERROR_NOT_INITIALIZED;
-  } else {
-    nrv = gCertVerificationThreadPool->Dispatch(job, NS_DISPATCH_NORMAL);
-  }
+  nsresult nrv = gCertVerificationThreadPool->Dispatch(job, NS_DISPATCH_NORMAL);
   if (NS_FAILED(nrv)) {
     // We can't call SetCertVerificationResult here to change
     // mCertVerificationState because SetCertVerificationResult will call
     // libssl functions that acquire SSL locks that are already being held at
-    // this point. infoObject->mCertVerificationState will be stuck at
-    // waiting_for_cert_verification here, but that is OK because we already
-    // have to be able to handle cases where we encounter non-cert errors while
-    // in that state.
+    // this point. However, we can set an error with PR_SetError and return
+    // SECFailure, and the correct thing will happen (the error will be
+    // propagated and this connection will be terminated).
     PRErrorCode error = nrv == NS_ERROR_OUT_OF_MEMORY
-                      ? SEC_ERROR_NO_MEMORY
+                      ? PR_OUT_OF_MEMORY_ERROR
                       : PR_INVALID_STATE_ERROR;
-    PORT_SetError(error);
+    PR_SetError(error, 0);
     return SECFailure;
   }
 
-  PORT_SetError(PR_WOULD_BLOCK_ERROR);
+  PR_SetError(PR_WOULD_BLOCK_ERROR, 0);
   return SECWouldBlock;
 }
 
 NS_IMETHODIMP
 SSLServerCertVerificationJob::Run()
 {
   // Runs on a cert verification thread
 
--- a/security/manager/ssl/moz.build
+++ b/security/manager/ssl/moz.build
@@ -187,9 +187,18 @@ for var in ('DLL_PREFIX', 'DLL_SUFFIX'):
 if not CONFIG['MOZ_SYSTEM_NSS']:
     USE_LIBS += [
         'crmf',
     ]
 
 include('/ipc/chromium/chromium-config.mozbuild')
 
 if CONFIG['GNU_CXX']:
-    CXXFLAGS += ['-Wno-error=shadow']
+    CXXFLAGS += [
+        '-Wextra',
+        # -Wextra enables this warning, but it's too noisy to be useful.
+        '-Wno-missing-field-initializers',
+    ]
+
+    # Gecko headers aren't warning-free enough for us to enable these warnings.
+    CXXFLAGS += [
+        '-Wno-unused-parameter',
+    ]