1235308 - Fix -Wimplicit-fallthrough warnings in security/

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: security_MOZ_FALLTHROUGH.patch

security/certverifier/NSSCertDBTrustDomain.cpp:282:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
security/manager/ssl/nsNSSComponent.cpp:149:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
security/manager/ssl/nsSecureBrowserUIImpl.cpp:1406:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels

MOZ_FALLTHROUGH (bug 1215411) is an annotation to suppress -Wimplicit-fallthrough warnings about switch cases that intentionally fall through without a break or return statement.

MOZ_FALLTHROUGH_ASSERT (bug 1235277) will suppress -Wimplicit-fallthrough warnings about switch cases that MOZ_ASSERT(false) (or its alias MOZ_ASSERT_UNREACHABLE) in debug builds, but intentionally fall through in release builds.

Why do we need MOZ_FALLTHROUGH_ASSERT in addition to MOZ_FALLTHROUGH? In release builds, the MOZ_ASSERT(false) will expand to `do { } while (0)`, requiring a MOZ_FALLTHROUGH annotation to suppress a -Wimplicit-fallthrough warning. In debug builds, the MOZ_ASSERT(false) will expand to something like `if (true) { MOZ_CRASH(); }` and the MOZ_FALLTHROUGH annotation will cause a -Wunreachable-code warning. The MOZ_FALLTHROUGH_ASSERT macro breaks this warning stalemate.

Comment 1

[Chris Peterson [:cpeterson]]

Comment on attachment 8702207 [details] [diff] [review]
security_MOZ_FALLTHROUGH.patch

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +282,3 @@
>    }
> +
> +  PR_NOT_REACHED("we're not handling every OCSPFetching type");

btw, removing the default case here (and moving PR_NOT_REACHED after the switch) will allow clang and gcc to report -Wswitch warnings if this switch fails to handle all the enum values of NSSCertDBTrustDomain::OCSPFetching:

https://dxr.mozilla.org/mozilla-central/source/security/certverifier/NSSCertDBTrustDomain.h?from=OCSPFetching#57

Comment 2

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

Comment on attachment 8702207 [details] [diff] [review]
security_MOZ_FALLTHROUGH.patch

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

LGTM.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5c9493e4e9

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b5c9493e4e9