Closed
Bug 857076
Opened 11 years ago
Closed 11 years ago
fix various build warnings in security/manager/ssl/src/
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
6.86 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
900 bytes,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
1.24 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
3.23 KB,
patch
|
briansmith
:
review-
|
Details | Diff | Splinter Review |
969 bytes,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
969 bytes,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
The PR_LOG call here disappears in opt builds and GCC warns, suggesting braces around the empty statement.
Attachment #732317 -
Flags: review?(bsmith)
Assignee | ||
Comment 4•11 years ago
|
||
The comment here already explains why it's OK to not examine the return value of the CERT_* calls.
Attachment #732318 -
Flags: review?(bsmith)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
contentLength here is signed; make it signed to satisfy MSVC.
Attachment #732321 -
Flags: review?(bsmith)
Comment 7•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #732316 -
Flags: review?(bsmith) → review+
Updated•11 years ago
|
Attachment #732317 -
Flags: review?(bsmith) → review+
Comment 8•11 years ago
|
||
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 9•11 years ago
|
||
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 10•11 years ago
|
||
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+
Comment 11•11 years ago
|
||
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
Comment 12•11 years ago
|
||
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.
Description
•