Closed Bug 1346315 Opened 5 years ago Closed 5 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

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 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.
Attachment #8853646 - Flags: review?(dkeeler)
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 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.
Attachment #8853646 - Flags: review?(dkeeler) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/11a29a22159a
Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/. r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/11a29a22159a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.