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

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: 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

Comment 2

[Dana Keeler (she/her) [:keeler]]

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.

Comment 3

[Dana Keeler (she/her) [:keeler]]

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.

Comment 4

[Dana Keeler (she/her) [:keeler]]

Oh, Brian reassigned the review. I'll assume he's fine with what I said, then.

Comment 5

[Daniel Holbert [:dholbert]]

(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.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: 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)

Comment 7

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=e4e130270e6f

Comment 8

[Daniel Holbert [:dholbert]]

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. :))

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ff203ccf025