1005309 - Enable more compiler warnings for mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Let's try to enable -Wall and (for VS2013) -W4. This should catch most subtle integer type conversion mistakes and also things like "if (x = y)" instead of "if (x == y)".

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Wrap calls to PK11_HashBuf with extra bounds checks and required typecast

The static_cast from unsigned to signed type is required. Note that eventually I want to move HashBuf to the TrustDomain as part of removing the hard-coded dependencies on NSS; this is also somewhat of a baby step towards that.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Enable -Wall and (on Windows) -W4 for mozilla::pkix

The changes to pkixocsp.cpp are needed to avoid signed/unsigned conversion warnings on VS2013.

I didn't do the same for the test code because GTest and other things make it more complicated to do so.

https://tbpl.mozilla.org/?tree=Try&rev=504ddba31484

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8416712 [details] [diff] [review]
Enable -Wall and (on Windows) -W4 for mozilla::pkix

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

dummy.c(2) : fatal error C1083: Cannot open include file: 'pthread.h': No such file or directory
c:\PROGRA~2\MICROS~2.0\vc\include\stdint.h(105) : error C2220: warning treated as error - no 'object' file generated

::: security/pkix/moz.build
@@ +25,5 @@
> +CXXFLAGS += ['-Wall']
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows':
> +    # C4514: 'function': unreferenced inline function has been removed
> +    # C4820: 'bytes' bytes padding added after construct 'member_name'
> +    CXXFLAGS += ['-W4', '-wd4514', '-wd4820']

Windows is red.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks Monica. Let's try again:
https://tbpl.mozilla.org/?tree=Try&rev=832c262dc76f

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8416710 [details] [diff] [review]
Part 1: Wrap calls to PK11_HashBuf with extra bounds checks and required typecast

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a939ec127378

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Enable -Wall on all platforms with some specific warnings disabled on MSVC++

Try run is green now:
https://tbpl.mozilla.org/?tree=Try&rev=b0ad262f6941

I also read the documentation and found that /Wall enables everything /W4 enables plus more, so I removed the /W4 part of the patch. See http://msdn.microsoft.com/en-us/library/thxezb7y.aspx

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8416977 [details] [diff] [review]
Part 2: Enable -Wall on all platforms with some specific warnings disabled on MSVC++

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

::: security/pkix/moz.build
@@ +29,5 @@
> +    '-wd4514', # 'function': unreferenced inline function has been removed
> +    '-wd4668', # 'symbol' is not defined as a preprocessor macro...
> +    '-wd4710', # 'function': function not inlined
> +    '-wd4711', # function 'function' selected for inline expansion
> +    '-wd4820', # 'bytes' bytes padding added after construct 'member_name'    

watch out for pink. Thanks for including this list!

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8416977 [details] [diff] [review]
Part 2: Enable -Wall on all platforms with some specific warnings disabled on MSVC++

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c584aaabfb5

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8416977 [details] [diff] [review]
Part 2: Enable -Wall on all platforms with some specific warnings disabled on MSVC++

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

::: security/pkix/moz.build
@@ +29,5 @@
> +    '-wd4514', # 'function': unreferenced inline function has been removed
> +    '-wd4668', # 'symbol' is not defined as a preprocessor macro...
> +    '-wd4710', # 'function': function not inlined
> +    '-wd4711', # function 'function' selected for inline expansion
> +    '-wd4820', # 'bytes' bytes padding added after construct 'member_name'    

Fixed before pushing. Thanks for the review.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a939ec127378
https://hg.mozilla.org/mozilla-central/rev/3c584aaabfb5

Comment 11

[Jacek Caban]

moz.build should test for compiler, not the target platform, for compiler-specific flags. I landed a trivial fixup (fixes mingw build):

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc9635af8e9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

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

Jacek, next time something like this happens, it's probably best to file a new bug (and get review or at least need-info the appropriate people) so that everyone who works on this code is aware of issues like this. As it is, the author of this patch will probably have no idea anything was wrong because this is likely to get lost in bugmail.

Comment 14

[Jacek Caban]

I assumed that this falls into 'trivial one-line fixup' not worth bothering reviewer. Apparently I was wrong and I'm sorry about that.

For the record: While the only tier 1 compiler for Windows is MSVC, it's also possible to build with mingw, which uses GCC. Those have very different compiler arguments. |CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows'| tests that we're doing a build for Windows, it says nothing about the compiler, so it will try to use MSVC-specific options on GCC as well. The right thing is to test for compiler instead.

Comment 15

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

You're right - I just meant in terms of us doing the right thing the first time in the future. Thanks for working on this and the information about windows/mingw :)