Closed Bug 968348 Opened 10 years ago Closed 10 years ago

Fix some 'unused' build warnings in security/manager/ssl/src/

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

In a Try run with security/manager/ssl/src tagged as FAIL_ON_WARNINGS, I ran across a few "unused variable" build warnings there (treated as errors in my Try run).

In particular, from the Linux builds:
{
../../../../../security/manager/ssl/src/SSLServerCertVerification.cpp:505:13: error: variable 'srv' set but not used [-Werror=unused-but-set-variable]
../../../../../security/manager/ssl/src/nsNSSCertificate.cpp:826:13: error: variable 'srv' set but not used [-Werror=unused-but-set-variable]
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=34093621&tree=Try

...and from the mac opt build:
{
../../../../../../security/manager/ssl/src/nsUsageArrayHelper.cpp:135:16: error: unused variable 'errorString' [-Werror,-Wunused-variable]
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=34094078&tree=Try

Patch coming up to fix these.
Attached patch fix v1Splinter Review
We could just drop the 'srv' variables, but I'm assuming they're there so that we can inspect their values in a debugger (and to make it clearer that we know there's a return value, and we're ignoring it. There's an explicit comment to that effect in CreateCertErrorRunnable).

I'm not 100% sure whether it's kosher to ignore it in the other spot (in nsNSSCertificate::GetChain), but that's what we're doing currently (and spamming a warning about).

And for the 'errorString' one, we can just drop the variable entirely and inline the PR_ErrorToName call (as we do elsewhere in some other logging macros e.g. [1] [2] [3])

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsDiskCacheStreams.cpp#147
[2] http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#7136
[3] http://mxr.mozilla.org/mozilla-central/source/storage/test/test_deadlock_detector.cpp#184
Attachment #8370926 - Flags: review?(brian)
Blocks: buildwarning
Depends on: 968363
Comment on attachment 8370926 [details] [diff] [review]
fix v1

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +533,5 @@
>    // 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.
> +  unused << srv;

My suggestion would be to move the comment explaining why we're ignoring the return value of certVerifier.VerifyCert to above the call, and then don't even declare/assign srv.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +856,5 @@
>                                     usage, PR_Now(),
>                                     nullptr, /*XXX fixme*/
>                                     CertVerifier::FLAG_LOCAL_ONLY,
>                                     &nssChain);
> +    unused << srv;

We completely ignore srv here - I don't think we even need to declare/assign it (really what we're looking for is if nssChain gets populated). This function is poorly specified and I intend to remove it/improve the interface in the relatively near future.

::: security/manager/ssl/src/nsUsageArrayHelper.cpp
@@ +143,5 @@
>    }
>  
>    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
>            ("error validating certificate for usage %s: %s (%d) -> %ud \n",
> +          typestr.get(), PR_ErrorToName(error), (int) error, (int) result));

LGTM.
Blocks: 968363
No longer depends on: 968363
Attachment #8370926 - Flags: review?(brian) → review?(dkeeler)
Comment on attachment 8370926 [details] [diff] [review]
fix v1

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

r=me with the changes I suggested, unless Brian disagrees.
Attachment #8370926 - Flags: review?(dkeeler) → review+
Oh, Brian reassigned the review. I'll assume he's fine with what I said, then.
(In reply to David Keeler (:keeler) from comment #2)
> We completely ignore srv here - I don't think we even need to declare/assign
> it (really what we're looking for is if nssChain gets populated).

Cool; I'll drop those 'srv' values & move the comment as you suggested.

(per comment 1, I was leaving it there in case it's useful to inspect with a debugger; but it sounds like it's not worth it.)
For reference, here's the updated patch that I intend to land, with "qdiff -w" to remove whitespace-only changes (from de-indenting subsequent lines of VerifyCert function-calls)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff203ccf025

(after pushing, I noticed a minor typo in my just-landed commit message: "an inline" should say "and inline". But, not a big enough deal to backout & repush. :))
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/6ff203ccf025
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: