Repository: randombit/botan
Pull request #3997 (2024-04-10 08:17:02)

Add helper for defining binary option pairs
Inspired by #3996 in the hope of preventing future errors of this type.


Patch:
diff --git a/configure.py b/configure.py
index 2d539bdd9..8789019ed 100755
--- a/configure.py
+++ b/configure.py
@@ -318,6 +318,30 @@ def process_command_line(args):
     available before logging is setup.
     """
 
+    def define_option_pair(group, verb, nverb, what, default, msg=optparse.SUPPRESS_HELP):
+        dest = '%s_%s' % (verb, what.replace('-', '_'))
+
+        # always show the help for the option that switches away from the default
+        with_help = msg if not default else optparse.SUPPRESS_HELP
+        without_help = msg if default else optparse.SUPPRESS_HELP
+
+        group.add_option('--%s-%s' % (verb, what),
+                         dest=dest,
+                         action='store_true',
+                         default=default,
+                         help=with_help)
+
+        group.add_option('--%s-%s' % (nverb, what),
+                         dest=dest,
+                         action='store_false',
+                         help=without_help)
+
+    def add_with_without_pair(group, what, default, msg=optparse.SUPPRESS_HELP):
+        define_option_pair(group, 'with', 'without', what, default, msg)
+
+    def add_enable_disable_pair(group, what, default, msg=optparse.SUPPRESS_HELP):
+        define_option_pair(group, 'enable', 'disable', what, default, msg)
+
     parser = optparse.OptionParser(
         formatter=optparse.IndentedHelpFormatter(max_help_position=50),
         version=Version.as_string())
@@ -371,12 +395,6 @@ def process_command_line(args):
     target_group.add_option('--compiler-cache',
                             help='specify a compiler cache to use')
 
-    target_group.add_option('--with-compilation-database', dest='with_compilation_database',
-                            action='store_true', default=True, help=optparse.SUPPRESS_HELP)
-
-    target_group.add_option('--without-compilation-database', dest='with_compilation_database',
-                            action='store_false', help='disable generation of compile_commands.json')
-
     target_group.add_option('--with-endian', metavar='ORDER', default=None,
                             help='override byte order guess')
 
@@ -385,15 +403,7 @@ def process_command_line(args):
     target_group.add_option('--without-os-features', action='append', metavar='FEAT',
                             help='specify OS features to disable')
 
-    target_group.add_option('--enable-experimental-features', dest='enable_experimental_features',
-                            action='store_true', default=False, help='enable building of experimental features and modules')
-    target_group.add_option('--disable-experimental-features', dest='enable_experimental_features',
-                            action='store_false', help=optparse.SUPPRESS_HELP)
-
-    target_group.add_option('--enable-deprecated-features', dest='enable_deprecated_features',
-                            action='store_true', default=True, help=optparse.SUPPRESS_HELP)
-    target_group.add_option('--disable-deprecated-features', dest='enable_deprecated_features',
-                            action='store_false', help='disable building of deprecated features and modules')
+    add_with_without_pair(target_group, 'compilation-database', True, 'disable compile_commands.json')
 
     isa_extensions = [
         'SSE2', 'SSSE3', 'SSE4.1', 'SSE4.2', 'AVX2', 'BMI2', 'RDRAND', 'RDSEED',
@@ -414,26 +424,16 @@ def process_command_line(args):
     build_group.add_option('--system-cert-bundle', metavar='PATH', default=None,
                            help='set path to trusted CA bundle')
 
-    build_group.add_option('--with-debug-info', action='store_true', default=False, dest='with_debug_info',
-                           help='include debug symbols')
+    add_with_without_pair(build_group, 'debug-info', False, 'include debug symbols')
 
-    build_group.add_option('--with-sanitizers', action='store_true', default=False, dest='with_sanitizers',
-                           help='enable ASan/UBSan checks')
+    add_with_without_pair(build_group, 'sanitizers', False, 'enable ASan/UBSan checks')
 
     build_group.add_option('--enable-sanitizers', metavar='SAN', default='',
                            help='enable specific sanitizers')
 
-    build_group.add_option('--with-stack-protector', dest='with_stack_protector',
-                           action='store_true', default=None, help=optparse.SUPPRESS_HELP)
-
-    build_group.add_option('--without-stack-protector', dest='with_stack_protector',
-                           action='store_false', help='disable stack smashing protections')
-
-    build_group.add_option('--with-coverage', action='store_true', default=False, dest='with_coverage',
-                           help='add coverage info and disable opts')
+    add_with_without_pair(build_group, 'stack-protector', True, 'disable stack smashing protections')
 
-    build_group.add_option('--with-coverage-info', action='store_true', default=False, dest='with_coverage_info',
-                           help='add coverage info')
+    add_with_without_pair(build_group, 'coverage-info', False, 'add coverage info')
 
     build_group.add_option('--enable-shared-library', dest='build_shared_lib',
                            action='store_true', default=None,
@@ -518,11 +518,10 @@ def process_command_line(args):
                            default=True, action='store_false',
                            help=optparse.SUPPRESS_HELP)
 
-    build_group.add_option('--with-valgrind', help='use valgrind API',
-                           dest='with_valgrind', action='store_true', default=False)
+    add_with_without_pair(build_group, 'valgrind', False, 'use valgrind API')
 
     build_group.add_option('--unsafe-fuzzer-mode', action='store_true', default=False,
-                           help='Disable essential checks for testing')
+                           help=optparse.SUPPRESS_HELP)
 
     build_group.add_option('--build-fuzzers', dest='build_fuzzers',
                            metavar='TYPE', default=None,
@@ -531,8 +530,7 @@ def process_command_line(args):
     build_group.add_option('--with-fuzzer-lib', metavar='LIB', default=None, dest='fuzzer_lib',
                            help='additionally link in LIB')
 
-    build_group.add_option('--with-debug-asserts', action='store_true', default=False,
-                           help=optparse.SUPPRESS_HELP)
+    add_with_without_pair(build_group, 'debug-asserts', default=False)
 
     build_group.add_option('--unsafe-terminate-on-asserts', action='store_true', default=False,
                            help=optparse.SUPPRESS_HELP)
@@ -543,48 +541,20 @@ def process_command_line(args):
     build_group.add_option('--build-tool', default='make',
                            help="specify the build tool (make, ninja)")
 
-    build_group.add_option('--with-pkg-config', action='store_true', default=None,
-                           help=optparse.SUPPRESS_HELP)
-    build_group.add_option('--without-pkg-config', dest='with_pkg_config', action='store_false',
-                           help=optparse.SUPPRESS_HELP)
-
-    build_group.add_option('--with-cmake-config', action='store_true', default=True,
-                           help=optparse.SUPPRESS_HELP)
-    build_group.add_option('--without-cmake-config', dest='with_cmake_config', action='store_false',
-                           help=optparse.SUPPRESS_HELP)
+    add_with_without_pair(build_group, 'pkg-config', default=None)
+    add_with_without_pair(build_group, 'cmake-config', default=True)
 
     docs_group = optparse.OptionGroup(parser, 'Documentation Options')
 
-    docs_group.add_option('--with-documentation', action='store_true',
-                          help=optparse.SUPPRESS_HELP)
-
-    docs_group.add_option('--without-documentation', action='store_false',
-                          default=True, dest='with_documentation',
-                          help='Skip building/installing documentation')
-
-    docs_group.add_option('--with-sphinx', action='store_true',
-                          default=None, help='Use Sphinx')
+    add_with_without_pair(docs_group, 'documentation', True, 'skip building/installing documentation')
 
-    docs_group.add_option('--without-sphinx', action='store_false',
-                          dest='with_sphinx', help=optparse.SUPPRESS_HELP)
+    add_with_without_pair(docs_group, 'sphinx', None, 'run Sphinx to generate docs')
 
-    docs_group.add_option('--with-pdf', action='store_true',
-                          default=False, help='Use Sphinx to generate PDF doc')
+    add_with_without_pair(docs_group, 'pdf', False, 'run Sphinx to generate PDF doc')
 
-    docs_group.add_option('--without-pdf', action='store_false',
-                          dest='with_pdf', help=optparse.SUPPRESS_HELP)
+    add_with_without_pair(docs_group, 'rst2man', None, 'run rst2man to generate man page')
 
-    docs_group.add_option('--with-rst2man', action='store_true',
-                          default=None, help='Use rst2man to generate man page')
-
-    docs_group.add_option('--without-rst2man', action='store_false',
-                          dest='with_rst2man', help=optparse.SUPPRESS_HELP)
-
-    docs_group.add_option('--with-doxygen', action='store_true',
-                          default=False, help='Use Doxygen')
-
-    docs_group.add_option('--without-doxygen', action='store_false',
-                          dest='with_doxygen', help=optparse.SUPPRESS_HELP)
+    add_with_without_pair(docs_group, 'doxygen', False, 'run Doxygen')
 
     mods_group = optparse.OptionGroup(parser, 'Module selection')
 
@@ -603,6 +573,12 @@ def process_command_line(args):
     mods_group.add_option('--minimized-build', action='store_true', dest='no_autoload',
                           help='minimize build')
 
+    add_enable_disable_pair(mods_group, 'experimental-features', False,
+                            'enable building of experimental features and modules')
+
+    add_enable_disable_pair(mods_group, 'deprecated-features', True,
+                            'disable building of deprecated features and modules')
+
     # Should be derived from info.txt but this runs too early
     third_party = ['boost', 'bzip2', 'lzma', 'commoncrypto', 'sqlite3', 'zlib', 'tpm']
 
@@ -690,10 +666,6 @@ def process_command_line(args):
         options.no_optimizations = True
         options.with_debug_info = True
 
-    if options.with_coverage:
-        options.with_coverage_info = True
-        options.no_optimizations = True
-
     def parse_multiple_enable(modules):
         if modules is None:
             return []
@@ -2221,7 +2193,7 @@ def create_template_vars(source_paths, build_paths, options, modules, disabled_m
         'build_fuzzers': options.build_fuzzers,
         'build_examples': 'examples' in options.build_targets,
 
-        'build_coverage' : options.with_coverage_info or options.with_coverage,
+        'build_coverage' : options.with_coverage_info,
 
         'symlink_shared_lib': options.build_shared_lib and osinfo.shared_lib_uses_symlinks,
 
diff --git a/doc/building.rst b/doc/building.rst
index 6cdd8a7b6..5abc50c2f 100644
--- a/doc/building.rst
+++ b/doc/building.rst
@@ -836,15 +836,10 @@ Enable specific sanitizers. See ``src/build-data/cc`` for more information.
 
 Disable stack smashing protections. **not recommended**
 
-``--with-coverage``
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-Add coverage info and disable optimizations
-
 ``--with-coverage-info``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-Add coverage info, but leave optimizations alone
+Add coverage info
 
 ``--disable-shared-library``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/src/lib/filters/buf_filt.cpp b/src/lib/filters/buf_filt.cpp
index 2a72bf73d..e73cca57b 100644
--- a/src/lib/filters/buf_filt.cpp
+++ b/src/lib/filters/buf_filt.cpp
@@ -9,6 +9,7 @@
 
 #include <botan/exceptn.h>
 #include <botan/mem_ops.h>
+#include <botan/internal/rounding.h>
 
 namespace Botan {
 
@@ -45,10 +46,8 @@ void Buffered_Filter::write(const uint8_t input[], size_t input_size) {
       input += to_copy;
       input_size -= to_copy;
 
-      const size_t available = std::min(m_buffer_pos, m_buffer_pos + input_size - m_final_minimum);
-
-      // Size down to available block size
-      const size_t total_to_consume = available - (available % m_main_block_mod);
+      size_t total_to_consume =
+         round_down(std::min(m_buffer_pos, m_buffer_pos + input_size - m_final_minimum), m_main_block_mod);
 
       buffered_block(m_buffer.data(), total_to_consume);
 
diff --git a/src/lib/utils/rounding.h b/src/lib/utils/rounding.h
index 68ecb5219..dd5ac9aa2 100644
--- a/src/lib/utils/rounding.h
+++ b/src/lib/utils/rounding.h
@@ -13,27 +13,31 @@
 namespace Botan {
 
 /**
-* Integer rounding
-*
-* Returns an integer z such that n <= z <= n + align_to
-* and z % align_to == 0
-*
-* @param n an integer
+* Round up
+* @param n a non-negative integer
 * @param align_to the alignment boundary
 * @return n rounded up to a multiple of align_to
 */
 inline size_t round_up(size_t n, size_t align_to) {
-   // Arguably returning n in this case would also be sensible
    BOTAN_ARG_CHECK(align_to != 0, "align_to must not be 0");
 
-   if(n % align_to > 0) {
-      const size_t adj = align_to - (n % align_to);
-      BOTAN_ARG_CHECK(n + adj >= n, "Integer overflow during rounding");
-      n += adj;
+   if(n % align_to) {
+      n += align_to - (n % align_to);
    }
    return n;
 }
 
+/**
+* Round down
+* @param n an integer
+* @param align_to the alignment boundary
+* @return n rounded down to a multiple of align_to
+*/
+template <typename T>
+inline constexpr T round_down(T n, T align_to) {
+   return (align_to == 0) ? n : (n - (n % align_to));
+}
+
 }  // namespace Botan
 
 #endif


Test patch:
diff --git a/src/tests/data/util.vec b/src/tests/data/util.vec
new file mode 100644
index 000000000..a1e857b10
--- /dev/null
+++ b/src/tests/data/util.vec
@@ -0,0 +1,58 @@
+
+[round_up]
+In1 = 1
+In2 = 10
+Out = 10
+
+In1 = 3
+In2 = 10
+Out = 10
+
+In1 = 9
+In2 = 10
+Out = 10
+
+In1 = 10
+In2 = 10
+Out = 10
+
+In1 = 1
+In2 = 4
+Out = 4
+
+In1 = 3
+In2 = 4
+Out = 4
+
+In1 = 4
+In2 = 4
+Out = 4
+
+In1 = 9
+In2 = 4
+Out = 12
+
+In1 = 11
+In2 = 4
+Out = 12
+
+In1 = 0
+In2 = 2
+Out = 0
+
+In1 = 0
+In2 = 10000
+Out = 0
+
+[round_down]
+In1 = 9
+In2 = 10
+Out = 0
+
+In1 = 10
+In2 = 10
+Out = 10
+
+In1 = 11
+In2 = 10
+Out = 10
diff --git a/src/tests/test_utils.cpp b/src/tests/test_utils.cpp
index d3917852f..16a52c137 100644
--- a/src/tests/test_utils.cpp
+++ b/src/tests/test_utils.cpp
@@ -33,54 +33,44 @@ namespace Botan_Tests {
 
 namespace {
 
-class Utility_Function_Tests final : public Test {
+class Utility_Function_Tests final : public Text_Based_Test {
    public:
-      std::vector<Test::Result> run() override {
-         std::vector<Test::Result> results;
-
-         results.push_back(test_round_up());
-         results.push_back(test_loadstore());
-         results.push_back(test_loadstore_fallback());
-         results.push_back(test_loadstore_constexpr());
-         return Botan::concat(results, test_copy_out_be_le());
-         return results;
-      }
+      Utility_Function_Tests() : Text_Based_Test("util.vec", "In1,In2,Out") {}
 
-   private:
-      Test::Result test_round_up() {
-         Test::Result result("Util round_up");
+      Test::Result run_one_test(const std::string& algo, const VarMap& vars) override {
+         Test::Result result("Util " + algo);
 
-         // clang-format off
-         const std::vector<size_t> inputs = {
-            0, 1, 2, 3, 4, 9, 10, 32, 99, 100, 101, 255, 256, 1000, 10000,
-            65535, 65536, 65537,
-         };
+         if(algo == "round_up") {
+            const size_t x = vars.get_req_sz("In1");
+            const size_t to = vars.get_req_sz("In2");
 
-         const std::vector<size_t> alignments = {
-            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 16, 32, 50, 64, 100, 512, 521,
-            1000, 1023, 1024, 1025, 10000, 65535, 65536
-         };
-         // clang-format on
+            result.test_eq(algo, Botan::round_up(x, to), vars.get_req_sz("Out"));
 
-         for(size_t i : inputs) {
-            for(size_t m : alignments) {
-               try {
-                  const size_t z = Botan::round_up(i, m);
+            try {
+               Botan::round_up(x, 0);
+               result.test_failure("round_up did not reject invalid input");
+            } catch(std::exception&) {}
+         } else if(algo == "round_down") {
+            const size_t x = vars.get_req_sz("In1");
+            const size_t to = vars.get_req_sz("In2");
 
-                  result.confirm("z % m == 0", z % m == 0);
-                  result.confirm("z >= i", z >= i);
-                  result.confirm("z <= i + m", z <= i + m);
-               } catch(Botan::Exception& e) {
-                  result.test_failure(Botan::fmt("round_up({},{})", i, m), e.what());
-               }
-            }
+            result.test_eq(algo, Botan::round_down<size_t>(x, to), vars.get_req_sz("Out"));
+            result.test_eq(algo, Botan::round_down<size_t>(x, 0), x);
          }
 
-         result.test_throws("Integer overflow is detected", []() { Botan::round_up(static_cast<size_t>(-1), 1024); });
-
          return result;
       }
 
+      std::vector<Test::Result> run_final_tests() override {
+         std::vector<Test::Result> results;
+
+         results.push_back(test_loadstore());
+         results.push_back(test_loadstore_fallback());
+         results.push_back(test_loadstore_constexpr());
+         return Botan::concat(results, test_copy_out_be_le());
+      }
+
+   private:
       using TestInt64 = Botan::Strong<uint64_t, struct TestInt64_>;
       using TestInt32 = Botan::Strong<uint32_t, struct TestInt64_>;
       using TestVectorSink = Botan::Strong<std::vector<uint8_t>, struct TestVectorSink_>;