1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[:Cykesiopka]

We enabled -Wextra for security/certverifier/ for gcc/clang in Bug 1026589.
We should do the same for the other PSM directories as well for added safety.

Notes:
1. -Wall is already enabled by default for all directories for GCC and clang.
2. Enabling the equivalent -Wall for MSVC will be done later.

Comment 1

[:Cykesiopka]

Attachment: Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/.

-Wextra implies -Wmissing-field-initializers, but since the latter warning seems
to warn about mostly uninteresting instances (XPCOM module definitions etc), we
disable it for now.

(Note that -Wall is already enabled by default for all directories for gcc and
clang.)

Review commit: https://reviewboard.mozilla.org/r/125742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125742/

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8853646 [details]
Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/.

https://reviewboard.mozilla.org/r/125742/#review128650

This looks good. I think we can make the section in SSLServerCertVerification better overall, though, so I suggested some changes there. If that sounds good, I'll re-review that and then we'll be good to go here.

::: security/manager/pki/moz.build:31
(Diff revision 1)
>  FINAL_LIBRARY = 'xul'
> +
> +if CONFIG['GNU_CXX']:
> +    CXXFLAGS += [
> +        '-Wextra',
> +        '-Wno-missing-field-initializers',

Maybe we should comment here that this is too noisy to be useful? (unless I've misunderstood the issue...)

::: security/manager/ssl/SSLServerCertVerification.cpp:1564
(Diff revision 1)
>      // have to be able to handle cases where we encounter non-cert errors while
>      // in that state.
>      PRErrorCode error = nrv == NS_ERROR_OUT_OF_MEMORY
> -                      ? SEC_ERROR_NO_MEMORY
> +                      ? PR_OUT_OF_MEMORY_ERROR
>                        : PR_INVALID_STATE_ERROR;
>      PORT_SetError(error);

This whole section needs a bit of re-working:
* we should use PR_SetError instead of PORT_SetError
* we should probably check gCertVerificationThreadPool early on, set the error to INVALID_STATE or LIBRARY_FAILURE and return SECFailure
* the comment seems a little outdated - I think it's intended to say something like "we can't call SetCertVerificationResult (because...), but we can set an error with PR_SetError and return SECFailure and the correct thing will happen (that an error was encountered will be propagated and this connection will be terminated)"

::: security/manager/ssl/moz.build:195
(Diff revision 1)
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
>  if CONFIG['GNU_CXX']:
> -    CXXFLAGS += ['-Wno-error=shadow']
> +    CXXFLAGS += [
> +        '-Wextra',
> +        '-Wno-missing-field-initializers',

Same idea with comment about why we're disabling this.

Comment 3

[:Cykesiopka]

Comment on attachment 8853646 [details]
Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/.

https://reviewboard.mozilla.org/r/125742/#review128650

Thanks.

> Maybe we should comment here that this is too noisy to be useful? (unless I've misunderstood the issue...)

Yeah, so far it mostly seems to catch places where we intentionally leave fields uninitialised because they're optional (XPCOM module definitions and so on). I'll look into turning it on later, but it doesn't seem useful enough to turn on at this point.

Comment 4

[:Cykesiopka]

Comment on attachment 8853646 [details]
Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125742/diff/1-2/

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8853646 [details]
Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/.

https://reviewboard.mozilla.org/r/125742/#review128966

Great - thanks.

Comment 6

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed97176307e092c1da534052497e4f086ea68750

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11a29a22159a
Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/. r=keeler

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11a29a22159a