Repository: randombit/botan
Pull request #3998 (2024-04-10 10:24:38)

Cleanup of rounding functionality
The round_down function was used exactly once - just remove it.

Improve testing of round_up, and detect integer overflows. This could only happen with very large alignment values, and these are always small constants in practice.


Patch:
diff --git a/src/lib/filters/buf_filt.cpp b/src/lib/filters/buf_filt.cpp
index e73cca57b..2a72bf73d 100644
--- a/src/lib/filters/buf_filt.cpp
+++ b/src/lib/filters/buf_filt.cpp
@@ -9,7 +9,6 @@
 
 #include <botan/exceptn.h>
 #include <botan/mem_ops.h>
-#include <botan/internal/rounding.h>
 
 namespace Botan {
 
@@ -46,8 +45,10 @@ void Buffered_Filter::write(const uint8_t input[], size_t input_size) {
       input += to_copy;
       input_size -= to_copy;
 
-      size_t total_to_consume =
-         round_down(std::min(m_buffer_pos, m_buffer_pos + input_size - m_final_minimum), m_main_block_mod);
+      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);
 
       buffered_block(m_buffer.data(), total_to_consume);
 
diff --git a/src/lib/utils/rounding.h b/src/lib/utils/rounding.h
index dd5ac9aa2..68ecb5219 100644
--- a/src/lib/utils/rounding.h
+++ b/src/lib/utils/rounding.h
@@ -13,31 +13,27 @@
 namespace Botan {
 
 /**
-* Round up
-* @param n a non-negative integer
+* Integer rounding
+*
+* Returns an integer z such that n <= z <= n + align_to
+* and z % align_to == 0
+*
+* @param n an 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) {
-      n += align_to - (n % align_to);
+   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;
    }
    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
deleted file mode 100644
index a1e857b10..000000000
--- a/src/tests/data/util.vec
+++ /dev/null
@@ -1,58 +0,0 @@
-
-[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 16a52c137..d3917852f 100644
--- a/src/tests/test_utils.cpp
+++ b/src/tests/test_utils.cpp
@@ -33,44 +33,54 @@ namespace Botan_Tests {
 
 namespace {
 
-class Utility_Function_Tests final : public Text_Based_Test {
+class Utility_Function_Tests final : public Test {
    public:
-      Utility_Function_Tests() : Text_Based_Test("util.vec", "In1,In2,Out") {}
+      std::vector<Test::Result> run() override {
+         std::vector<Test::Result> results;
 
-      Test::Result run_one_test(const std::string& algo, const VarMap& vars) override {
-         Test::Result result("Util " + algo);
+         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;
+      }
 
-         if(algo == "round_up") {
-            const size_t x = vars.get_req_sz("In1");
-            const size_t to = vars.get_req_sz("In2");
+   private:
+      Test::Result test_round_up() {
+         Test::Result result("Util round_up");
 
-            result.test_eq(algo, Botan::round_up(x, to), vars.get_req_sz("Out"));
+         // 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,
+         };
 
-            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");
+         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_down<size_t>(x, to), vars.get_req_sz("Out"));
-            result.test_eq(algo, Botan::round_down<size_t>(x, 0), x);
-         }
+         for(size_t i : inputs) {
+            for(size_t m : alignments) {
+               try {
+                  const size_t z = Botan::round_up(i, m);
 
-         return result;
-      }
+                  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());
+               }
+            }
+         }
 
-      std::vector<Test::Result> run_final_tests() override {
-         std::vector<Test::Result> results;
+         result.test_throws("Integer overflow is detected", []() { Botan::round_up(static_cast<size_t>(-1), 1024); });
 
-         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 result;
       }
 
-   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_>;