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

RESOLVED FIXED in mozilla30

Status

()

Core
Security
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Created attachment 8370926 [details] [diff] [review]
fix v1

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: 187528
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.)
Created attachment 8371253 [details] [diff] [review]
updated patch (no-whitespace version)

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)
Try run: https://tbpl.mozilla.org/?tree=Try&rev=e4e130270e6f
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.