Merge GH #2206 Start building GCC/Clang with -Werror in CI

12 files changed, 56 insertions, 25 deletions

diff --git a/configure.py b/configure.py
index 9f50677df..2c04dbc90 100755
--- a/configure.py
+++ b/configure.py
@@ -448,6 +448,10 @@ def process_command_line(args): # pylint: disable=too-many-locals,too-many-state
                            action='store_true', default=False,
                            help="Enable extra warnings")
 
+    build_group.add_option('--werror-mode', dest='werror_mode',
+                           action='store_true', default=False,
+                           help="Prohibit compiler warnings")
+
     build_group.add_option('--no-store-vc-rev', action='store_true', default=False,
                            help=optparse.SUPPRESS_HELP)
 
@@ -1153,6 +1157,7 @@ class CompilerInfo(InfoObject): # pylint: disable=too-many-instance-attributes
                 'ar_command': '',
                 'ar_options': '',
                 'ar_output_to': '',
+                'werror_flags': '',
             })
 
         self.add_framework_option = lex.add_framework_option
@@ -1192,6 +1197,7 @@ class CompilerInfo(InfoObject): # pylint: disable=too-many-instance-attributes
         self.visibility_attribute = lex.visibility_attribute
         self.visibility_build_flags = lex.visibility_build_flags
         self.warning_flags = lex.warning_flags
+        self.werror_flags = lex.werror_flags
 
     def cross_check(self, os_info, arch_info, all_isas):
 
@@ -1362,6 +1368,8 @@ class CompilerInfo(InfoObject): # pylint: disable=too-many-instance-attributes
     def cc_warning_flags(self, options):
         def gen_flags():
             yield self.warning_flags
+            if options.werror_mode or options.maintainer_mode:
+                yield self.werror_flags
             if options.maintainer_mode:
                 yield self.maintainer_warning_flags
 
diff --git a/src/build-data/cc/clang.txt b/src/build-data/cc/clang.txt
index e0b9d3a33..b183f60ef 100644
--- a/src/build-data/cc/clang.txt
+++ b/src/build-data/cc/clang.txt
@@ -5,7 +5,10 @@ binary_name clang++
 lang_flags "-std=c++11 -D_REENTRANT"
 
 warning_flags "-Wall -Wextra -Wpedantic -Wshadow -Wstrict-aliasing -Wstrict-overflow=5 -Wcast-align -Wmissing-declarations -Wpointer-arith -Wcast-qual"
-maintainer_warning_flags "-Wunreachable-code -Wdocumentation -Qunused-arguments -Werror -Wno-error=unused-parameter -Wno-error=unreachable-code -Wno-error=unused-lambda-capture"
+
+werror_flags "-Werror -Wno-error=unused-parameter -Wno-error=unreachable-code -Wno-error=unused-lambda-capture"
+
+maintainer_warning_flags "-Wunreachable-code -Wdocumentation -Qunused-arguments"
 
 optimization_flags "-O3"
 sanitizer_optimization_flags "-O1 -fno-optimize-sibling-calls -fno-omit-frame-pointer"
diff --git a/src/build-data/cc/gcc.txt b/src/build-data/cc/gcc.txt
index 7393358a6..4ff005bd5 100644
--- a/src/build-data/cc/gcc.txt
+++ b/src/build-data/cc/gcc.txt
@@ -7,7 +7,9 @@ lang_flags "-std=c++11 -D_REENTRANT"
 # This should only contain flags which are included in GCC 4.8
 warning_flags "-Wall -Wextra -Wpedantic -Wstrict-aliasing -Wcast-align -Wmissing-declarations -Wpointer-arith -Wcast-qual -Wzero-as-null-pointer-constant -Wnon-virtual-dtor"
 
-maintainer_warning_flags "-Wstrict-overflow=5 -Wold-style-cast -Wsuggest-override -Wshadow -Wextra-semi -Werror -Wno-error=strict-overflow"
+werror_flags "-Werror -Wno-error=strict-overflow -Wno-error=zero-as-null-pointer-constant"
+
+maintainer_warning_flags "-Wstrict-overflow=5 -Wold-style-cast -Wsuggest-override -Wshadow -Wextra-semi"
 
 optimization_flags "-O3"
 sanitizer_optimization_flags "-O1 -fno-optimize-sibling-calls -fno-omit-frame-pointer"
diff --git a/src/build-data/cc/msvc.txt b/src/build-data/cc/msvc.txt
index 2306a4485..a7dc9bbb0 100644
--- a/src/build-data/cc/msvc.txt
+++ b/src/build-data/cc/msvc.txt
@@ -25,7 +25,14 @@ debug_info_flags "/Zi /FS"
 preproc_flags "/nologo /EP"
 
 lang_flags "/EHs /GR /D_ENABLE_EXTENDED_ALIGNED_STORAGE"
-warning_flags "/W4 /wd4250 /wd4251 /wd4275"
+
+# 4250: diamond inheritence warning
+# 4251: STL types used in DLL interface
+# 4275: ???
+# 4127: conditional expression is constant, consider using if constexpr
+warning_flags "/W4 /wd4250 /wd4251 /wd4275 /wd4127"
+
+werror_flags "/WX"
 
 visibility_build_flags "/DBOTAN_DLL=__declspec(dllexport)"
 visibility_attribute "__declspec(dllimport)"
diff --git a/src/cli/speed.cpp b/src/cli/speed.cpp
index 6449c33c4..0593fc1b5 100644
--- a/src/cli/speed.cpp
+++ b/src/cli/speed.cpp
@@ -1836,7 +1836,7 @@ class Speed final : public Command
          if(invalid_sigs > 0)
             error_output() << invalid_sigs << " generated signatures rejected in PK signature bench\n";
 
-         const size_t events = std::min(sig_timer->events(), ver_timer->events());
+         const size_t events = static_cast<size_t>(std::min(sig_timer->events(), ver_timer->events()));
 
          record_result(sig_timer);
          record_result(ver_timer);
diff --git a/src/lib/pubkey/xmss/xmss_privatekey.cpp b/src/lib/pubkey/xmss/xmss_privatekey.cpp
index 6a728ac72..72bb0c06f 100644
--- a/src/lib/pubkey/xmss/xmss_privatekey.cpp
+++ b/src/lib/pubkey/xmss/xmss_privatekey.cpp
@@ -29,18 +29,18 @@ namespace {
 
 // fall back to raw decoding for previous versions, which did not encode an OCTET STRING
 secure_vector<uint8_t> extract_raw_key(const secure_vector<uint8_t>& key_bits)
-{
+   {
    secure_vector<uint8_t> raw_key;
    try
-   {
+      {
       BER_Decoder(key_bits).decode(raw_key, OCTET_STRING);
-   }
-   catch(Decoding_Error& e)
-   {
+      }
+   catch(Decoding_Error&)
+      {
       raw_key = key_bits;
-   }
+      }
    return raw_key;
-}
+   }
 
 }
 
@@ -116,6 +116,7 @@ XMSS_PrivateKey::tree_hash(size_t start_idx,
                            size_t target_node_height,
                            XMSS_Address& adrs)
    {
+   BOTAN_ASSERT_NOMSG(target_node_height <= 30);
    BOTAN_ASSERT((start_idx % (1 << target_node_height)) == 0,
                 "Start index must be divisible by 2^{target node height}.");
 
@@ -137,7 +138,8 @@ XMSS_PrivateKey::tree_hash(size_t start_idx,
    const size_t subtrees = static_cast<size_t>(1) << split_level;
    const size_t last_idx = (static_cast<size_t>(1) << (target_node_height)) + start_idx;
    const size_t offs = (last_idx - start_idx) / subtrees;
-   uint8_t level = split_level; // current level in the tree
+   // this cast cannot overflow because target_node_height is limited
+   uint8_t level = static_cast<uint8_t>(split_level); // current level in the tree
 
    BOTAN_ASSERT((last_idx - start_idx) % subtrees == 0,
                 "Number of worker threads in tree_hash need to divide range "
diff --git a/src/lib/pubkey/xmss/xmss_publickey.cpp b/src/lib/pubkey/xmss/xmss_publickey.cpp
index 43f7417cf..c19a2f972 100644
--- a/src/lib/pubkey/xmss/xmss_publickey.cpp
+++ b/src/lib/pubkey/xmss/xmss_publickey.cpp
@@ -25,18 +25,18 @@ namespace {
 
 // fall back to raw decoding for previous versions, which did not encode an OCTET STRING
 std::vector<uint8_t> extract_raw_key(const std::vector<uint8_t>& key_bits)
-{
+   {
    std::vector<uint8_t> raw_key;
    try
-   {
+      {
       BER_Decoder(key_bits).decode(raw_key, OCTET_STRING);
-   }
-   catch(Decoding_Error& e)
-   {
+      }
+   catch(Decoding_Error&)
+      {
       raw_key = key_bits;
-   }
+      }
    return raw_key;
-}
+   }
 
 }
 
diff --git a/src/lib/utils/http_util/http_util.cpp b/src/lib/utils/http_util/http_util.cpp
index 3fd08b5b8..804b1d2c6 100644
--- a/src/lib/utils/http_util/http_util.cpp
+++ b/src/lib/utils/http_util/http_util.cpp
@@ -195,7 +195,8 @@ Response http_sync(http_exch_fn http_transact,
    while(io.good())
       {
       io.read(cast_uint8_ptr_to_char(buf.data()), buf.size());
-      resp_body.insert(resp_body.end(), buf.data(), &buf[io.gcount()]);
+      const size_t got = static_cast<size_t>(io.gcount());
+      resp_body.insert(resp_body.end(), buf.data(), &buf[got]);
       }
 
    const std::string header_size = search_map(headers, std::string("Content-Length"));
diff --git a/src/lib/utils/mem_ops.h b/src/lib/utils/mem_ops.h
index 8878603fc..ea811ee50 100644
--- a/src/lib/utils/mem_ops.h
+++ b/src/lib/utils/mem_ops.h
@@ -132,7 +132,10 @@ template<typename T> inline void clear_mem(T* ptr, size_t n)
 template<typename T> inline void copy_mem(T* out, const T* in, size_t n)
    {
    static_assert(std::is_trivial<typename std::decay<T>::type>::value, "");
-   if(n > 0)
+   BOTAN_ASSERT_IMPLICATION(n > 0, in != nullptr && out != nullptr,
+                            "If n > 0 then args are not null");
+
+   if(in != nullptr && out != nullptr && n > 0)
       {
       std::memmove(out, in, sizeof(T)*n);
       }
diff --git a/src/scripts/ci_build.py b/src/scripts/ci_build.py
index b75b52e84..b212f33bc 100755
--- a/src/scripts/ci_build.py
+++ b/src/scripts/ci_build.py
@@ -70,6 +70,9 @@ def determine_flags(target, target_os, target_cpu, target_cc, cc_bin, ccache, ro
              '--os=%s' % (target_os)]
     build_targets = ['cli', 'tests']
 
+    if target_cc in ['gcc', 'clang'] and target != 'gcc4.8':
+        flags += ['--werror-mode']
+
     if target_cpu is not None:
         flags += ['--cpu=%s' % (target_cpu)]
 
diff --git a/src/tests/main.cpp b/src/tests/main.cpp
index d6815bd31..6f511bcf2 100644
--- a/src/tests/main.cpp
+++ b/src/tests/main.cpp
@@ -111,7 +111,7 @@ int main(int argc, char* argv[])
 
       int rc = tests.run(opts);
 
-#if defined(BOTAN_HAS_OPENSSL)
+#if defined(BOTAN_HAS_OPENSSL) && defined(OPENSSL_VERSION_NUMBER) && (OPENSSL_VERSION_NUMBER < 0x01010000)
       if(opts.provider().empty() || opts.provider() == "openssl")
          {
          ERR_free_strings();
diff --git a/src/tests/test_pkcs11_low_level.cpp b/src/tests/test_pkcs11_low_level.cpp
index 6cf3dcdd0..89492210c 100644
--- a/src/tests/test_pkcs11_low_level.cpp
+++ b/src/tests/test_pkcs11_low_level.cpp
@@ -616,8 +616,8 @@ std::array<Attribute, 4> dtemplate =
       {
          { static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Class), &object_class, sizeof(object_class) },
          { static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Token), &btrue, sizeof(btrue) },
-         { static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Label), const_cast< char* >(label.c_str()), label.size() },
-         { static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Value), const_cast< char* >(data.c_str()), data.size() }
+         { static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Label), const_cast<char*>(label.c_str()), static_cast<CK_ULONG>(label.size()) },
+         { static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Value), const_cast<char*>(data.c_str()), static_cast<CK_ULONG>(data.size()) }
       }
    };
 
@@ -766,7 +766,9 @@ Test::Result test_c_copy_object()
 
    std::string copied_label = "A copied data object";
 
-   Attribute copy_attribute_values = { static_cast< CK_ATTRIBUTE_TYPE >(AttributeType::Label), const_cast< char* >(copied_label.c_str()), copied_label.size() };
+   Attribute copy_attribute_values = {
+      static_cast<CK_ATTRIBUTE_TYPE>(AttributeType::Label), const_cast<char*>(copied_label.c_str()), static_cast<CK_ULONG>(copied_label.size())
+   };
 
    auto binder = std::bind(&LowLevel::C_CopyObject, *p11_low_level.get(), session_handle, object_handle,
                            &copy_attribute_values, 1, &copied_object_handle, std::placeholders::_1);