fable: Fix {N,D}::validate_bounds() incorrect validation

This happened when incorrectly casting from signed to unsigned.
Tests have been added to test these edge cases.

Author: cassava

fable/include/fable/schema/duration.hpp

@@ -29,7 +29,8 @@
 #include <string>   // for string
 #include <utility>  // for move
 
-#include <fable/schema/interface.hpp>  // for Base<>
+#include <fable/schema/interface.hpp>   // for Base<>
+#include <fable/utility/templates.hpp>  // for is_safe_cast<>, typeinfo<>
 
 namespace fable::schema {
 
@@ -171,34 +172,36 @@ class Duration : public Base<Duration<T, Period>> {
    */
   template <typename B>
   bool validate_bounds(const Conf& c, std::optional<SchemaError>& err) const {
-    auto v = c.get<B>();
-    if (!std::numeric_limits<B>::is_signed && value_min_ < 0) {
-      // If B is unsigned and value_min_ is less than 0, there is no way
-      // that v cannot fulfill the minimum requirements. Trying to use the
-      // other branches will "underflow" the value_min_ which will invalidate
-      // any comparison.
-    } else if (exclusive_min_) {
-      if (v <= static_cast<B>(value_min_)) {
-        return this->set_error(err, c, "expected exclusive minimum of {}, got {}", value_min_, v);
+    auto original = c.get<B>();
+    auto value = static_cast<T>(original);
+    if constexpr (!std::is_floating_point_v<T>) {
+      if (!is_cast_safe<T>(original)) {
+        return this->set_error(err, c,
+                               "failed to convert input to destination type {}, got {}( {} ) = {}",
+                               typeinfo<T>::name, typeinfo<B>::name, original, value);
+      }
+    }
+
+    // Check minimum value:
+    if (exclusive_min_) {
+      if (value <= value_min_) {
+        return this->set_error(err, c, "expected exclusive minimum of {}, got {}", value_min_,
+                               value);
       }
     } else {
-      if (v < static_cast<B>(value_min_)) {
-        return this->set_error(err, c, "expected minimum of {}, got {}", value_min_, v);
+      if (value < value_min_) {
+        return this->set_error(err, c, "expected minimum of {}, got {}", value_min_, value);
       }
     }
 
-    if (!std::numeric_limits<B>::is_signed && value_max_ < 0) {
-      // If B is unsigned, but our maximum value is somewhere below 0, then v
-      // will by definition always be out-of-bounds.
-      return this->set_error(err, c, "expected {}maximum of {}, got {}",
-                             (exclusive_max_ ? "exclusive " : ""), value_max_, v);
-    } else if (exclusive_max_) {
-      if (v >= static_cast<B>(value_max_)) {
-        return this->set_error(err, c, "expected exclusive maximum of {}, got {}", value_max_, v);
+    if (exclusive_max_) {
+      if (value >= value_max_) {
+        return this->set_error(err, c, "expected exclusive maximum of {}, got {}", value_max_,
+                               value);
       }
     } else {
-      if (v > static_cast<B>(value_max_)) {
-        return this->set_error(err, c, "expected maximum of {}, got {}", value_max_, v);
+      if (value > value_max_) {
+        return this->set_error(err, c, "expected maximum of {}, got {}", value_max_, value);
       }
     }
 

fable/include/fable/schema/number_impl.hpp

@@ -34,7 +34,8 @@
 #include <utility>           // for move
 #include <vector>            // for vector<>
 
-#include <fable/schema/number.hpp>  // for Number<>
+#include <fable/schema/number.hpp>      // for Number<>
+#include <fable/utility/templates.hpp>  // for is_safe_cast<>, typeinfo<>
 
 namespace fable::schema {
 
@@ -273,49 +274,58 @@ void Number<T>::reset_ptr() {
 
 /**
  * Check that the min and max bounds are held by c.
+ *
+ * Use-cases with the bounds T={} and the input B=[]:
+ * (a) ---{---[--0--]---}--- B within T
+ *     Note that this doesn't mean we can stop looking, since the actual range
+ *     might be in the top or bottom of T:
+ *     -------[--0--]{--}---
+ *
+ * (b) ---[---{--0--}---]--- T within B
+ *
+ * (c) ---[------{------]--} T overlaps B
  */
 template <typename T>
 template <typename B>
 bool Number<T>::validate_bounds(const Conf& c, std::optional<SchemaError>& err) const {
-  auto v = c.get<B>();
+  auto original = c.get<B>();
+  auto value = static_cast<T>(original);
+  if constexpr (!std::is_floating_point_v<T>) {
+    if (!is_cast_safe<T>(original)) {
+      return this->set_error(err, c,
+                             "failed to convert input to destination type {}, got {}( {} ) = {}",
+                             typeinfo<T>::name, typeinfo<B>::name, original, value);
+    }
+  }
 
-  // Check whitelist and blacklist first.
+  // Check whitelist and blacklist first:
   if (!std::is_floating_point_v<T>) {
-    if (whitelist_.count(v)) {
+    if (whitelist_.count(value)) {
       return true;
     }
-    if (blacklist_.count(v)) {
-      return this->set_error(err, c, "unexpected blacklisted value {}", v);
+    if (blacklist_.count(value)) {
+      return this->set_error(err, c, "unexpected blacklisted value {}", value);
     }
   }
 
-  if (!std::numeric_limits<B>::is_signed && value_min_ < 0) {
-    // If B is unsigned and value_min_ is less than 0, there is no way
-    // that v cannot fulfill the minimum requirements. Trying to use the
-    // other branches will "underflow" the value_min_ which will invalidate
-    // any comparison.
-  } else if (exclusive_min_) {
-    if (v <= static_cast<B>(value_min_)) {
-      return this->set_error(err, c, "expected exclusive minimum of {}, got {}", value_min_, v);
+  // Check minimum value:
+  if (exclusive_min_) {
+    if (value <= value_min_) {
+      return this->set_error(err, c, "expected exclusive minimum > {}, got {}", value_min_, value);
     }
   } else {
-    if (v < static_cast<B>(value_min_)) {
-      return this->set_error(err, c, "expected minimum of {}, got {}", value_min_, v);
+    if (value < value_min_) {
+      return this->set_error(err, c, "expected minimum >= {}, got {}", value_min_, value);
     }
   }
 
-  if (!std::numeric_limits<B>::is_signed && value_max_ < 0) {
-    // If B is unsigned, but our maximum value is somewhere below 0, then v
-    // will by definition always be out-of-bounds.
-    return this->set_error(err, c, "expected {}maximum of {}, got {}",
-                           (exclusive_max_ ? "exclusive " : ""), value_max_, v);
-  } else if (exclusive_max_) {
-    if (v >= static_cast<B>(value_max_)) {
-      return this->set_error(err, c, "expected exclusive maximum of {}, got {}", value_max_, v);
+  if (exclusive_max_) {
+    if (value >= value_max_) {
+      return this->set_error(err, c, "expected exclusive maximum < {}, got {}", value_max_, value);
     }
   } else {
-    if (v > static_cast<B>(value_max_)) {
-      return this->set_error(err, c, "expected maximum of {}, got {}", value_max_, v);
+    if (value > value_max_) {
+      return this->set_error(err, c, "expected maximum <= {}, got {}", value_max_, value);
     }
   }
 

fable/include/fable/utility/templates.hpp

@@ -0,0 +1,82 @@
+/*
+ * Copyright 2023 Robert Bosch GmbH
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+/**
+ * \file fable/utility/number.hpp
+ */
+
+#pragma once
+
+#include <cstdint> // for int8_t, ...
+#include <limits>  // for numeric_limits
+
+namespace fable {
+
+/**
+ * Return true if casting value to target type is safe.
+ *
+ * Safe means the specific value provided will not have its sign or value changed
+ * by the conversion.
+ */
+template <typename T, typename S>
+constexpr bool is_cast_safe(S value) {
+  if (static_cast<S>(static_cast<T>(value)) != value) {
+    // Ensure no narrowing is occurring.
+    return false;
+  }
+
+  if constexpr (std::numeric_limits<T>::is_signed != std::numeric_limits<S>::is_signed) {
+    // Mismatch in signedness can go wrong in two ways that aren't caught above.
+    if constexpr (std::numeric_limits<S>::is_signed) {
+      // Check value will not underflow.
+      return value >= 0;
+    } else {
+      // Check value will not overflow.
+      return static_cast<unsigned long long>(value) <=
+             static_cast<unsigned long long>(std::numeric_limits<T>::max());
+    }
+  }
+
+  return true;
+}
+
+/**
+ * Encode the string name of a type as a static member.
+ * This is used for error messages.
+ *
+ * Example:
+ *
+ *     std::cout << typeinfo<int8_t>::name << std::endl;
+ */
+template <typename T>
+struct typeinfo {
+  static const constexpr char* name = "unknown";
+};
+
+// clang-format off
+template <> struct typeinfo<bool>     { static const constexpr char* name = "bool"; };
+template <> struct typeinfo<int8_t>   { static const constexpr char* name = "int8_t"; };
+template <> struct typeinfo<int16_t>  { static const constexpr char* name = "int16_t"; };
+template <> struct typeinfo<int32_t>  { static const constexpr char* name = "int32_t"; };
+template <> struct typeinfo<int64_t>  { static const constexpr char* name = "int64_t"; };
+template <> struct typeinfo<uint8_t>  { static const constexpr char* name = "uint8_t"; };
+template <> struct typeinfo<uint16_t> { static const constexpr char* name = "uint16_t"; };
+template <> struct typeinfo<uint32_t> { static const constexpr char* name = "uint32_t"; };
+template <> struct typeinfo<uint64_t> { static const constexpr char* name = "uint64_t"; };
+// clang-format on
+
+}  // namespace fable

fable/src/fable/schema/number_test.cpp

@@ -26,12 +26,13 @@ using namespace std;  // NOLINT(build/namespaces)
 
 #include <gtest/gtest.h>
 
-#include <fable/confable.hpp>       // for Confable
-#include <fable/schema.hpp>         // for Number
-#include <fable/utility/gtest.hpp>  // for assert_validate
+#include <fable/confable.hpp>           // for Confable
+#include <fable/schema.hpp>             // for Number
+#include <fable/utility/gtest.hpp>      // for assert_validate
+#include <fable/utility/templates.hpp>  // for typeinfo
 
 struct MyNumberStruct : public fable::Confable {
-  uint8_t number;
+  uint8_t number = 0;
   CONFABLE_SCHEMA(MyNumberStruct) {
     using namespace fable::schema;  // NOLINT(build/namespace)
     return Struct{
@@ -76,3 +77,71 @@ TEST(fable_schema_number, validate) {
                                   });
   }
 }
+
+template <typename T>
+void assert_validate_t(const fable::Schema& schema, const std::vector<T>& xs) {
+  for (T x : xs) {
+    std::cerr << "Expect ok   " << fable::typeinfo<T>::name << ":\t" << (+x) << std::endl;
+    fable::assert_validate(schema, fable::Json(x));
+  }
+}
+
+template <typename T>
+void assert_invalidate_t(const fable::Schema& schema, const std::vector<T>& xs) {
+  for (T x : xs) {
+    std::cerr << "Expect fail " << fable::typeinfo<T>::name << ":\t" << (+x) << std::endl;
+    fable::assert_invalidate(schema, fable::Json(x));
+  }
+}
+
+TEST(fable_schema_number, validate_u64) {
+  auto schema = fable::schema::Number<uint64_t>(nullptr, "");
+
+  assert_validate_t<uint64_t>(schema, {0, 1, std::numeric_limits<uint64_t>::max()});
+  assert_validate_t<int64_t>(schema, {0, 1, std::numeric_limits<int64_t>::max()});
+
+  assert_invalidate_t<int8_t>(schema, {-1, -5, std::numeric_limits<int8_t>::min()});
+  assert_invalidate_t<int64_t>(schema, {-1, -5, std::numeric_limits<int8_t>::min()});
+}
+
+TEST(fable_schema_number, validate_u64_narrow_high) {
+  auto schema = fable::schema::Number<uint64_t>(nullptr, "")
+                    .minimum(std::numeric_limits<uint64_t>::max() - 2);
+
+  assert_validate_t<uint64_t>(
+      schema, {std::numeric_limits<uint64_t>::max() - 2, std::numeric_limits<uint64_t>::max()});
+  assert_invalidate_t<uint64_t>(schema, {0, std::numeric_limits<uint64_t>::max() - 3});
+  assert_invalidate_t<uint32_t>(schema, {0, std::numeric_limits<uint32_t>::max()});
+  assert_invalidate_t<int32_t>(schema, {-1, std::numeric_limits<int32_t>::max()});
+}
+
+TEST(fable_schema_number, validate_u8) {
+  auto schema = fable::schema::Number<uint8_t>(nullptr, "");
+
+  assert_validate_t<uint8_t>(schema, {0, 1, std::numeric_limits<uint8_t>::max()});
+  assert_validate_t<int8_t>(schema, {0, 1, std::numeric_limits<int8_t>::max()});
+
+  assert_invalidate_t<uint32_t>(schema, {std::numeric_limits<uint32_t>::max()});
+  assert_invalidate_t<int8_t>(schema, {-1, -5, std::numeric_limits<int8_t>::min()});
+  assert_invalidate_t<int64_t>(schema, {-1, -5, std::numeric_limits<int8_t>::min()});
+}
+
+TEST(fable_schema_number, validate_i64) {
+  auto schema = fable::schema::Number<int64_t>(nullptr, "");
+
+  assert_validate_t<uint64_t>(schema, {0, 1, std::numeric_limits<uint64_t>::max() / 2});
+  assert_validate_t<int64_t>(schema, {0, 1, std::numeric_limits<int64_t>::max()});
+
+  assert_invalidate_t<uint64_t>(schema, {std::numeric_limits<uint64_t>::max()});
+}
+
+TEST(fable_schema_number, validate_i8) {
+  auto schema = fable::schema::Number<int8_t>(nullptr, "");
+
+  assert_validate_t<uint8_t>(schema, {0, 1, std::numeric_limits<uint8_t>::max() / 2});
+  assert_validate_t<int8_t>(schema, {-1, 0, 1, std::numeric_limits<int8_t>::max()});
+
+  assert_invalidate_t<uint64_t>(schema, {std::numeric_limits<uint32_t>::max()});
+  assert_invalidate_t<int64_t>(
+      schema, {std::numeric_limits<int16_t>::min(), std::numeric_limits<int64_t>::max()});
+}