1128413 - Enable more compiler warnings in mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: Part 1: Enable more warnings

See attached patches.

Comment 1

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

Attachment: Part 2: Stop using "__" in include guards

The current SVN version of clang (and probably recent versions like 3.7), when compiling with -Weverything, warn about identifiers containing "__" because they are apparently reserved in some versions of the ISO C standard. IMO, it shouldn't warn about this when compiling in C++ mode, but it's easier for me to adjust mozilla::pkix than to adjust clang.

Comment 2

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

Attachment: Part 4: Make mozilla-config.h and GCC STL wrappers compile without warnings with clang

The latest versions of clang (e.g. clang SVN) issue warnings for these files. In the case of mozilla-config.h, the issue is defining macros that contain "__", which is apparently reserved by some version of the ISO C standard in *any* part of the identifier, not just the beginning. In the case of the STL wrappers, the issue is the use of the __EXCEPTIONS macro when it isn't defined.

Comment 3

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

Attachment: Part 1: Enable more warnings [v2]

Fixed a minor issue caught on try run rebased on top of other changes.

Comment 4

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

Comment on attachment 8557757 [details] [diff] [review]
Part 1: Enable more warnings [v2]

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

::: security/pkix/include/pkix/Input.h
@@ +79,5 @@
>      , len(0u)
>    {
>    }
>  
> +  Input(const Input&) = default;

clang warns that implicit default copy constructors are deprecated when the default operator= is deleted.

@@ -270,5 @@
>    }
>  
>    Result SkipToEnd(/*out*/ Input& skipped)
>    {
> -    return Skip(static_cast<size_t>(end - input), skipped);

This was implicitly narrowing size_t to size_type, which is uint16_t.

::: security/pkix/lib/pkixcheck.cpp
@@ -444,5 @@
>                            Result::FATAL_ERROR_LIBRARY_FAILURE);
> -
> -      default:
> -        return NotReached("unrecognized EKU",
> -                          Result::FATAL_ERROR_LIBRARY_FAILURE);

Recent clang warns that this is unreachable code. (If a case were missing, it would warn about the missing case.)

::: security/pkix/lib/pkixtime.cpp
@@ +28,4 @@
>  #ifdef WIN32
> +#ifdef _MSC_VER
> +#pragma warning(push, 3)
> +#endif

Prevent C4668.

::: security/pkix/lib/pkixutil.h
@@ -52,5 @@
>  
>    Result Init();
>  
>    const Input GetDER() const { return der; }
> -  const der::Version GetVersion() const { return version; }

redundant const.

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ -109,5 @@
>                 : TrustLevel::InheritsTrust;
>      return Success;
>    }
>  
> -  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time time)

"time" is not referenced. Many of these test changes are to prevent the unreferenced named parameter warning.

@@ -342,5 @@
>    ScopedTestKeyPair reusedKey(CloneReusedKeyPair());
>    ByteString certDER(CreateEncodedCertificate(
>                         v3, sha256WithRSAEncryption,
>                         serialNumber, issuerDER,
> -                       oneDayBeforeNow - Time::ONE_DAY_IN_SECONDS,

clang (I think) doesn't like the implicit conversion to time_t.

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ -846,5 @@
>              NestedOf(input, SEQUENCE, INTEGER, EmptyAllowed::No,
>                       [&readValues](Reader& r) {
>                         return NestedOfHelper(r, readValues);
>                       }));
> -  ASSERT_EQ((size_t) 3, readValues.size());

clang doesn't like the old-style cast.

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ -287,5 @@
>  // other encodings is actually encouraged.
>  
>  // e.g. TWO_CHARS(53) => '5', '3'
> -#define TWO_CHARS(t) static_cast<uint8_t>('0' + ((t) / 10u)), \
> -                     static_cast<uint8_t>('0' + ((t) % 10u))

The changes in this file are about implicit signed/unsigned conversion/comparison.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +72,5 @@
>    { \
>      ByteString(reinterpret_cast<const uint8_t*>(a), sizeof(a) - 1), \
>      ByteString(reinterpret_cast<const uint8_t*>(b), sizeof(b) - 1), \
> +    Result::ERROR_BAD_DER, \
> +    false \

Recent clang warns when you don't initialize every member of a struct. The value of the forth field doesn't matter when the third field isn't Result::SUCCESS.

::: security/pkix/test/lib/pkixtestutil.h
@@ -167,5 @@
>  template <size_t L>
>  inline ByteString
>  RFC822Name(const char (&bytes)[L])
>  {
> -  return RFC822Name(ByteString(reinterpret_cast<const uint8_t (&)[L]>(bytes),

Clang says that this cast is undefined behavior.

Comment 5

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

Comment on attachment 8557757 [details] [diff] [review]
Part 1: Enable more warnings [v2]

in moz.build files, "#include" is a comment, not an include. I need to redo this patch.

Comment 6

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

Attachment: Part 1: Fix switch-related warnings

I recommend reading the changes to pkixutil.h first.

Comment 7

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

Attachment: Part 3: Enable more compiler warnings [v3]

tryserver run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5762ddbf8310

I also tested this with the tip-of-tree Clang.

Comment 8

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

Comment on attachment 8558245 [details] [diff] [review]
Part 3: Enable more compiler warnings [v3]

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

::: security/pkix/include/pkix/Input.h
@@ +79,5 @@
>      , len(0u)
>    {
>    }
>  
> +  Input(const Input&) = default;

Clang complains that implicit default copy constructors are deprecated when the assignment operator is deleted.

@@ -270,5 @@
>    }
>  
>    Result SkipToEnd(/*out*/ Input& skipped)
>    {
> -    return Skip(static_cast<size_t>(end - input), skipped);

implicit cast from size_t to size_type is lossy.

@@ +288,5 @@
>  
>    class Mark final
>    {
> +  public:
> +    Mark(const Mark&) = default;

Copy constructor must be explicitly defaulted when assignment operator is deleted.

::: security/pkix/lib/pkixnames.cpp
@@ +1649,4 @@
>            componentsToMove * 2u);
>    // Fill in the contracted area with zeros.
> +  memset(address + (2u * static_cast<size_t>(contractionIndex)), 0u,
> +         (8u - static_cast<size_t>(numComponents)) * 2u);

signed/unsigned.

@@ -1784,5 @@
>        // Contraction
>        if (contractionIndex != -1) {
>          return false; // multiple contractions are not allowed.
>        }
> -      uint8_t b;

This declaration of b shadows another declaration of "uint8_t b" in the containing scope.

::: security/pkix/lib/pkixtime.cpp
@@ +32,3 @@
>  #include "windows.h"
> +#ifdef _MSC_VER
> +#pragma warning(pop)

Microsoft only tries to get windows.h to compile without warnings using W3, not /W4 or higher.

@@ +60,5 @@
>    //   - http://pubs.opengroup.org/onlinepubs/009695399/functions/gettimeofday.html
>    timeval tv;
>    (void) gettimeofday(&tv, nullptr);
> +  seconds = (DaysBeforeYear(1970) * Time::ONE_DAY_IN_SECONDS) +
> +            static_cast<uint64_t>(tv.tv_sec);

signed/unsigned.

::: security/pkix/lib/pkixutil.h
@@ -52,5 @@
>  
>    Result Init();
>  
>    const Input GetDER() const { return der; }
> -  const der::Version GetVersion() const { return version; }

const is ignored here.

@@ +254,1 @@
>  #endif

Sorry, these changes were supposed to be folded into part 1. I will do so after the review.

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ -109,5 @@
>                 : TrustLevel::InheritsTrust;
>      return Success;
>    }
>  
> -  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time time)

Most of the warnings in the tests are either about unused parameters or signed/unsigned issues.

::: security/pkix/test/gtest/pkixcheck_CheckKeyUsage_tests.cpp
@@ +21,5 @@
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
>  
> +#include "pkixgtest.h"

pkixgtest.h wraps the include of gtest/gtest.h in some junk that prevents warnings, so we never include gtest/gtest.h directly anymore.

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ -864,5 @@
>              NestedOf(input, SEQUENCE, INTEGER, EmptyAllowed::No,
>                       [&readValues](Reader& r) {
>                         return NestedOfHelper(r, readValues);
>                       }));
> -  ASSERT_EQ((size_t) 0, readValues.size());

old-style cast.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +72,5 @@
>    { \
>      ByteString(reinterpret_cast<const uint8_t*>(a), sizeof(a) - 1), \
>      ByteString(reinterpret_cast<const uint8_t*>(b), sizeof(b) - 1), \
> +    Result::ERROR_BAD_DER, \
> +    false \

Clang hates it when you don't initialize all the fields of a struct, although in this case the value of the fourth field doesn't matter.

Comment 9

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

Comment on attachment 8558244 [details] [diff] [review]
Part 1: Fix switch-related warnings

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

::: security/pkix/lib/pkixnss.cpp
@@ +65,5 @@
> +#pragma clang diagnostic ignored "-Wswitch-enum"
> +#elif defined(_MSC_VER)
> +#pragma warning(push)
> +#pragma warning(disable: 4061)
> +#endif

This junk will get removed along with the the whole switch statement in bug 1122841.

@@ +117,5 @@
> +#if defined(__clang__)
> +#pragma clang diagnostic pop
> +#elif defined(_MSC_VER)
> +#pragma warning(pop)
> +#endif

Ditto.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8557748 [details] [diff] [review]
Part 4: Make mozilla-config.h and GCC STL wrappers compile without warnings with clang

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

::: mozilla-config.h.in
@@ +10,5 @@
> +#pragma clang diagnostic push
> +#if __has_warning("-Wreserved-id-macro")
> +#pragma clang diagnostic ignored "-Wreserved-id-macro"
> +#endif
> +#endif

If that is about this:
> In the case of mozilla-config.h, the issue is defining macros that contain "__"

The let's just get rid of those macros with "__". AFAICT, there's only MOZ_MSVC_STL_WRAP__Throw and MOZ_MSVC_STL_WRAP__RAISE. Both can be easily changed.

Comment 11

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

Comment on attachment 8557748 [details] [diff] [review]
Part 4: Make mozilla-config.h and GCC STL wrappers compile without warnings with clang

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

::: mozilla-config.h.in
@@ +10,5 @@
> +#pragma clang diagnostic push
> +#if __has_warning("-Wreserved-id-macro")
> +#pragma clang diagnostic ignored "-Wreserved-id-macro"
> +#endif
> +#endif

I am happy to make that change too. However, that isn't sufficient, because clang warns about __STDC_LIMIT_MACROS stuff below. Apparently C++11 doesn't need the __STDC_LIMIT_MACROS stuff any more so I'll try to remove it and see what happens.

Comment 12

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

Comment on attachment 8557748 [details] [diff] [review]
Part 4: Make mozilla-config.h and GCC STL wrappers compile without warnings with clang

I tried doing what you suggested, Mike, but it wasn't sufficient.

When we compile with Clang on Linux, we end up defining macros like these as well:

 HAVE__UNWIND_BACKTRACE
 HAVE___CXA_DEMANGLE
 _REENTRANT

As well as:

 __STDC_LIMIT_MACROS
 __STDC_CONSTANT_MACROS
 __STDC_FORMAT_MACROS

Also, I think that even if I fix all of those, if it is even feasible, it's still pretty brittle, compared to what I'm doing here. Thus, I'm re-requesting review.

Comment 13

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

I tried removing the __STDC_* macro definitions and it resulted in several build failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=870516ccd627

So, I think we're best off just leaving them alone, which would require the pragmas.

Comment 14

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

Comment on attachment 8558244 [details] [diff] [review]
Part 1: Fix switch-related warnings

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

If the comments below are invalid because you are covering all of the enums in the switch above, then please disregard. LGTM otherwise.

::: security/pkix/lib/pkixcheck.cpp
@@ +440,5 @@
>          break;
>  
>        case KeyPurposeId::anyExtendedKeyUsage:
>          return NotReached("anyExtendedKeyUsage should start with found==true",
>                            Result::FATAL_ERROR_LIBRARY_FAILURE);

Doesn't this require MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM?

::: security/pkix/lib/pkixnames.cpp
@@ +1134,5 @@
>        }
>        break;
>      }
>  
>      case IDRole::PresentedID: // fall through

"fall through" comment is incorrect

@@ +1339,5 @@
>        if (!constraintRDNs.AtEnd() || !presentedRDNs.AtEnd()) {
>          return Result::ERROR_CERT_NOT_IN_NAME_SPACE;
>        }
>        matches = true;
>        return Success;

I don't understand why this doesn't generate a compiler warning about not having a default case.

Comment 15

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

Comment on attachment 8558245 [details] [diff] [review]
Part 3: Enable more compiler warnings [v3]

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

LG pending green try. Thanks for fixing these up!

::: security/pkix/include/pkix/Input.h
@@ +80,5 @@
>    {
>    }
>  
> +  Input(const Input&) = default;
> +

Can this be marked explicit, or does that not matter because the assignment operator is deleted? Also does it need to be public or do we always use Init instead?

@@ +288,5 @@
>  
>    class Mark final
>    {
> +  public:
> +    Mark(const Mark&) = default;

same question as above

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +555,5 @@
>  };
>  
>  TEST_F(pkixder_universal_types_tests, TimeMonthDaysValidRange)
>  {
> +  for (int16_t month = 1; month <= 12; ++month) {

seems sad that months are not uint! same with year.

Comment 16

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

Attachment: Part 4(b): Use non-reserved names for MSVC throw/raise macros

Unfortunately, the rest of part 4 is still necessary because these are not the only reserved names we use (see the list above). However, I'd already written this patch when investigating that so we might as well land it too.

Comment 17

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

Attachment: Part 3: Enable more compiler warnings [v4]

Carrying over r+.

Comment 18

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

Comment on attachment 8560920 [details] [diff] [review]
Part 3: Enable more compiler warnings [v4]

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

::: security/pkix/include/pkix/Input.h
@@ +80,5 @@
>    {
>    }
>  
> +  // This is intentionally not explicit in order to allow value semantics.
> +  Input(const Input&) = default;

I added a comment to this constructor and to the Mark constructor about why they are not explicit. If you want to enable value semantics, then you shouldn't have the copy constructor be explicit.

::: security/pkix/lib/pkixutil.h
@@ +252,2 @@
>  #else
> +#error Unsupported compiler for MOZILLA_PKIX_UNREACHABLE_DEFAULT.

You mentioned that it was unclear why some cases don't require MOZILLA_PKIX_UNREACHABLE_DEFAULT. I only added MOZILLA_PKIX_UNREACHABLE_DEFAULT to switch statements that caused the compilers to warn if it wasn't present. Sometimes the compilers are either smart enough to figure out that no other cases are possible, or the compiler can otherwise see that there is no "use of uninitialized variable" or "not all code paths return a value" issue resulting from the lack of a default.

::: security/pkix/warnings.mozbuild
@@ +11,5 @@
> +    '-Wno-padded',
> +    '-Wno-reserved-id-macro', # XXX: Will be removed in bug 1128413, Part 4.
> +    '-Wno-shadow', # XXX: Clang's rules are too strict for constructors.
> +    '-Wno-undef', # XXX: Will be removed in bug 1128413, Part 4.
> +    '-Wno-weak-vtables', # We rely on the linker to merge the duplicate vtables.

I added -Wno-reserved-id-macro and -Wno-undef here so that this will build without part 4.

Comment 19

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

Attachment: Part 3(b): Fix more warnings reported by MSVC 2015

Comment 20

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

Comment on attachment 8560941 [details] [diff] [review]
Part 3(b): Fix more warnings reported by MSVC 2015

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

::: security/pkix/lib/pkixder.h
@@ +54,5 @@
>  {
>    CONSTRUCTED = 1 << 5
>  };
>  
> +enum Tag : uint8_t

Signed/unsigned warnings in files that use this type.

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +466,5 @@
>    ASSERT_TRUE(InputsAreEqual(expected, item));
>  }
>  
>  // Cannot run this test on debug builds because of the NotReached
> +#ifdef NDEBUG

When building mozilla::pkix outside of Gecko's build system, I learned that MSVC doesn't define DEBUG in debug builds. I also learned that NDEBUG is what we really need, since that controls how assert() works.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +131,3 @@
>    } else if (value.length() < 256) {
>      result.push_back(0x81u);
> +    result.push_back(static_cast<uint8_t>(value.length()));

signed/unsigned.

@@ +157,5 @@
>  
>    , certStatus(good)
>    , revocationTime(0)
>    , thisUpdate(time)
> +  , nextUpdate(time + static_cast<time_t>(Time::ONE_DAY_IN_SECONDS))

signed/unsigned.

@@ +201,5 @@
>  ByteString
>  Boolean(bool value)
>  {
>    ByteString encodedValue;
> +  encodedValue.push_back(value ? 0xffu : 0x00u);

signed/unsigned.

@@ +280,5 @@
> +  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_hour % 10)));
> +  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_min / 10)));
> +  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_min % 10)));
> +  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_sec / 10)));
> +  value.push_back(static_cast<uint8_t>('0' + (exploded.tm_sec % 10)));

signed/unsigned.

@@ +458,5 @@
> +    free(value);
> +  }
> +#endif
> +  return result;
> +}

MSVC hates getenv.

@@ +850,5 @@
>  //   critical         BOOLEAN DEFAULT FALSE
>  //   value            OCTET STRING
>  // }
>  static ByteString
> +OCSPExtension(OCSPResponseExtension& extension)

Unused parameter.

Comment 21

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

Attachment: Part 4(a): Suppress clang -Wundef and -Wreserved-id-macro warnings

Rebased on top of the new versions of part 3. See the comments above for an explanation of why I wasn't able to make your suggested changes from the previous review.

Comment 22

[Masatoshi Kimura [:emk]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)
> The current SVN version of clang (and probably recent versions like 3.7),
> when compiling with -Weverything, warn about identifiers containing "__"
> because they are apparently reserved in some versions of the ISO C standard.
> IMO, it shouldn't warn about this when compiling in C++ mode, but it's
> easier for me to adjust mozilla::pkix than to adjust clang.

Double underscore is always reserved even in C++11.
https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier
I haven't heard it was changed in C++14. Clang is perfectly correct about this. Rather, I have wondered why switches like -Wreserved-id-macro) were not introduced earlier.

Comment 23

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

Comment on attachment 8560941 [details] [diff] [review]
Part 3(b): Fix more warnings reported by MSVC 2015

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

These changes look fine. I have mixed feelings about adding a MSVC 2015 version of GetEnv, since it's not an officially supported build platform. Is there a plan for keeping these changes up to date?

Comment 24

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

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #23)
> These changes look fine. I have mixed feelings about adding a MSVC 2015
> version of GetEnv, since it's not an officially supported build platform. Is
> there a plan for keeping these changes up to date?

The GetEnv function code is shared between MSVC 2013 and MSVC 2015. It isn't 2015-specific.

Regarding the more general issue that MSVC 2015 isn't supported yet and that standalone builds of mozilla::pkix outside of Gecko aren't supported yet: When they break, I will submit patches to fix them.

Thanks for the reviews everybody!

Comment 25

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

Thanks again for the reviews:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9f7e2678bfb
https://hg.mozilla.org/integration/mozilla-inbound/rev/09c407963c95
https://hg.mozilla.org/integration/mozilla-inbound/rev/d17a125c8ee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/80a2f25792fc

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e9f7e2678bfb
https://hg.mozilla.org/mozilla-central/rev/09c407963c95
https://hg.mozilla.org/mozilla-central/rev/d17a125c8ee7
https://hg.mozilla.org/mozilla-central/rev/80a2f25792fc