Closed Bug 1128413 Opened 9 years ago Closed 9 years ago

Enable more compiler warnings in mozilla::pkix

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 4 obsolete files)

15.12 KB, patch
mmc
: review+
Details | Diff | Splinter Review
14.94 KB, patch
mmc
: review+
Details | Diff | Splinter Review
4.21 KB, patch
glandium
: review+
Details | Diff | Splinter Review
43.82 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
12.45 KB, patch
mmc
: review+
Details | Diff | Splinter Review
3.73 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Attached patch Part 1: Enable more warnings (obsolete) — Splinter Review
See attached patches.
Attachment #8557742 - Flags: review?(mmc)
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.
Attachment #8557744 - Flags: review?(mmc)
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.
Attachment #8557748 - Flags: review?(mh+mozilla)
Fixed a minor issue caught on try run rebased on top of other changes.
Attachment #8557742 - Attachment is obsolete: true
Attachment #8557742 - Flags: review?(mmc)
Attachment #8557757 - Flags: review?(mmc)
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 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.
Attachment #8557757 - Attachment is obsolete: true
Attachment #8557757 - Flags: review?(mmc)
Attachment #8557744 - Flags: review?(mmc) → review+
Attachment #8557748 - Attachment description: Part 3: Make mozilla-config.h and GCC STL wrappers compile without warnings with clang → Part 4: Make mozilla-config.h and GCC STL wrappers compile without warnings with clang
I recommend reading the changes to pkixutil.h first.
Attachment #8558244 - Flags: review?(mmc)
tryserver run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5762ddbf8310

I also tested this with the tip-of-tree Clang.
Attachment #8558245 - Flags: review?(mmc)
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 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 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.
Attachment #8557748 - Flags: review?(mh+mozilla) → feedback+
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 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.
Attachment #8557748 - Flags: review?(mh+mozilla)
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 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.
Attachment #8558244 - Flags: review?(mmc) → review+
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.
Attachment #8558245 - Flags: review?(mmc) → review+
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.
Attachment #8560016 - Flags: review?(mh+mozilla)
Carrying over r+.
Attachment #8558245 - Attachment is obsolete: true
Attachment #8560920 - Flags: review+
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 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.
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.
Attachment #8557748 - Attachment is obsolete: true
Attachment #8557748 - Flags: review?(mh+mozilla)
Attachment #8560942 - Flags: review?(mh+mozilla)
Attachment #8560016 - Flags: review?(mh+mozilla) → review+
Attachment #8560942 - Flags: review?(mh+mozilla) → review+
(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 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?
Attachment #8560941 - Flags: review?(mmc) → review+
(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!
You need to log in before you can comment on or make changes to this bug.