Closed Bug 857076 Opened 11 years ago Closed 11 years ago

fix various build warnings in security/manager/ssl/src/

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

I was attempting to make this directory FAIL_ON_WARNINGS, but that proved to be a bit difficult.  The forthcoming patch series makes the directory warning-free on at least Linux, Windows, and possibly Android.
Several variables are only used in (debug-only) assertions, producing unused
variable warnings in opt builds.  Wrap such variables in DebugOnly<> to quiet
the opt builds.
Attachment #732315 - Flags: review?(bsmith)
The compiler claims that it's possible for this variable to be used without
initialization.  I don't think that's true, because of how `srv' and `rv'
are set together, but it's difficult to tell.  And if it's difficult to tell,
we might as well just make the compiler happy.
Attachment #732316 - Flags: review?(bsmith)
The PR_LOG call here disappears in opt builds and GCC warns, suggesting
braces around the empty statement.
Attachment #732317 - Flags: review?(bsmith)
The comment here already explains why it's OK to not examine the return value
of the CERT_* calls.
Attachment #732318 - Flags: review?(bsmith)
mPlaintextBytesRead is a 64-bit variable and MSVC complains that we might be
losing some precision.  Losing some precision is OK here (and also unlikely).
Attachment #732319 - Flags: review?(bsmith)
contentLength here is signed; make it signed to satisfy MSVC.
Attachment #732321 - Flags: review?(bsmith)
Comment on attachment 732315 [details] [diff] [review]
part 1 - add DebugOnly<> annotations in security/manager/ssl/src/

Review of attachment 732315 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed.

::: security/manager/ssl/src/nsCMS.cpp
@@ +217,5 @@
>    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsCMSMessage::CommonVerifySignature, content level count %d\n", NSS_CMSMessage_ContentLevelCount(m_cmsMsg)));
>    NSSCMSContentInfo *cinfo = nullptr;
>    NSSCMSSignedData *sigd = nullptr;
>    NSSCMSSignerInfo *si;
> +  DebugOnly<int32_t> nsigners;

Instead of doing this, please change the PR_ASSERT(nsigners > 0); to NS_ENSURE_TRUE(nsigners > 0, NS_ERROR_UNEXPECTED);

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1051,5 @@
>      nsMyTrustedEVInfo &entry = myTrustedEVInfos[iEV];
>      if (!entry.oid_name) // invalid or placeholder list entry
>        continue;
>  
> +    DebugOnly<SECStatus> rv;

Again, PR_Assert here isn't even valid; it is possible (but very, very unlikely) for those functions to fail (with OOM from fallible allocators). So, we should change the assertions in this function to real checks.
Attachment #732315 - Flags: review?(bsmith) → review+
Attachment #732316 - Flags: review?(bsmith) → review+
Attachment #732317 - Flags: review?(bsmith) → review+
Comment on attachment 732318 [details] [diff] [review]
part 4 - delete set-but-unused `srv' variable in CreateCertErrorRunnable

Review of attachment 732318 [details] [diff] [review]:
-----------------------------------------------------------------

I'm guessing this is very bitrotten and needs to be redone.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +501,5 @@
> +  // We ignore the result code of the cert verification.
> +  // Either it is a failure, which is expected, and we'll process the
> +  //                         verify log below.
> +  // Or it is a success, then a domain mismatch is the only 
> +  //                     possible failure. 

Please fix the whitespace issues with this comment.

@@ +506,2 @@
>    if (!nsNSSComponent::globalConstFlagUsePKIXVerification) {
> +    CERT_VerifyCertificate(CERT_GetDefaultCertDB(), cert,

(void) CERT_VerifyCertificate

@@ +514,5 @@
>      cvout[0].type = cert_po_errorLog;
>      cvout[0].value.pointer.log = verify_log;
>      cvout[1].type = cert_po_end;
>  
> +    CERT_PKIXVerifyCert(cert, certificateUsageSSLServer,

(void) CERT_PKIXVerifyCert
Attachment #732318 - Flags: review?(bsmith) → review-
Comment on attachment 732319 [details] [diff] [review]
part 5 - add a uint32_t cast for Telemetry::Accumulate value in nsNSSIOLayer.cpp

Review of attachment 732319 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +460,5 @@
>    }
>  
>    if (mPlaintextBytesRead && !errorCode) {
> +    Telemetry::Accumulate(Telemetry::SSL_BYTES_BEFORE_CERT_CALLBACK,
> +                          uint32_t(mPlaintextBytesRead));

Please use SafeCast from mfbt/Casting.h
Attachment #732319 - Flags: review?(bsmith) → review+
Comment on attachment 732321 [details] [diff] [review]
part 6 - add a size_t cast to nsMemory::Alloc's argument

Review of attachment 732321 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +3022,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    
>    mBufferOffset = 0;
>    mBufferSize = 0;
> +  mByteData = (char*) nsMemory::Alloc(size_t(contentLength));

Please use SafeCast from mfbt/casting.h
Attachment #732321 - Flags: review?(bsmith) → review+
I pushed all the changes I r+d, with the slight modifications I asked for because other people were reporting partial duplicates of this bug and plus I keep tripping over the warnings now. There are some parts of the patches I didn't push; if you notice more warnings in PSM feel free to file more bugs. I want to squash them all.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae51db07aed4
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/ae51db07aed4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.