From f0b40ca4d4d44cd5f8e9e040b753847d147990a5 Mon Sep 17 00:00:00 2001 From: Eugen Betke Date: Tue, 8 Oct 2024 20:24:55 +0000 Subject: [PATCH] ECC-1930: Fix more overflows --- src/NumericLimits.h | 112 ++++++++++++------ src/accessor/grib_accessor_class_bits.cc | 4 +- .../grib_accessor_class_bufr_data_array.cc | 5 +- ...or_class_from_scale_factor_scaled_value.cc | 5 +- src/accessor/grib_accessor_class_unsigned.cc | 3 +- tests/grib_set_fail.sh | 2 +- 6 files changed, 88 insertions(+), 43 deletions(-) diff --git a/src/NumericLimits.h b/src/NumericLimits.h index b7cc9c7c1..0c9ac4db7 100644 --- a/src/NumericLimits.h +++ b/src/NumericLimits.h @@ -11,59 +11,99 @@ #pragma once #include +#include +#include // CHAR_BIT -template +// NumericLimits is a class template that provides the minimum and maximum values for a given number of bits. +// The minimum and maximum values are calculated for both signed and unsigned integral types. + +// Example: +// For a 16-bit signed integer, the minimum and maximum values are: +// nbits | min | max +// ----- | --- | --- +// 1 | -1 | 0 +// 2 | -2 | 1 +// 3 | -4 | 3 +// 4 | -8 | 7 +// ... +// 15 | -16384 | 16383 +// 16 | -32768 | 32767 + +template ::value, bool = std::is_integral::value> struct NumericLimits { - static_assert(std::is_integral::value, "ValueType must be an integral type"); - private: - using S = std::make_signed_t; - using U = std::make_unsigned_t; - static constexpr int MaxN = 8 * sizeof(ValueType); + static constexpr uint8_t MaxN = CHAR_BIT * sizeof(ValueType); static constexpr std::array max_ = []() { - if constexpr (std::is_signed::value) { - std::array max{}; - for (int i = 1; i < MaxN; ++i) { - auto ones = ~(static_cast(0)); - max[i] = static_cast(ones >> (MaxN - i)); - } - return max; - } - else { - std::array max{}; - for (int i = 1; i < MaxN; ++i) { - auto ones = ~(static_cast(0)); - max[i] = ones >> (MaxN - i - 1); - } - return max; + std::array max{}; + using UnsignedValueType = std::make_unsigned_t; + constexpr UnsignedValueType ones = ~UnsignedValueType{0}; + max[0] = 0; + for (uint8_t i = 1; i < MaxN; ++i) { + max[i] = static_cast(ones >> (MaxN - i)); } + return max; }(); static constexpr std::array min_ = []() { - if constexpr (std::is_signed::value) { - std::array min{}; - for (int i = 1; i < MaxN; ++i) { - min[i] = ~max_[i]; - } - return min; - } - else { - return std::array{}; + std::array min{}; + for (uint8_t i = 0; i < MaxN; ++i) { + min[i] = ~max_[i]; } + return min; }(); public: - static constexpr ValueType min(int nbits) + static constexpr ValueType min(uint8_t nbits) { - if constexpr (std::is_signed::value) - return min_[nbits - 1]; - else - return 0; + return min_[nbits - 1]; } - static constexpr ValueType max(int nbits) + static constexpr ValueType max(uint8_t nbits) + { + return max_[nbits - 1]; + } +}; + + +// Example: +// For a 16-bit unsigned integer, the minimum and maximum values are: +// nbits | min | max +// ----- | --- | --- +// 1 | 0 | 1 +// 2 | 0 | 3 +// 3 | 0 | 7 +// 4 | 0 | 15 +// ... +// 15 | 0 | 32767 +// 16 | 0 | 65535 + +template +struct NumericLimits +{ + static_assert(std::is_integral::value, "ValueType must be an integral type"); + +private: + static constexpr uint8_t MaxN = CHAR_BIT * sizeof(ValueType); + + static constexpr std::array max_ = []() { + std::array max{}; + constexpr ValueType ones = ~(static_cast(0)); + max[0] = 1; + for (uint8_t i = 1; i < MaxN; ++i) { + max[i] = ones >> (MaxN - i - 1); + } + return max; + }(); + +public: + static constexpr ValueType min(uint8_t nbits) + { + return 0; + } + + static constexpr ValueType max(uint8_t nbits) { return max_[nbits - 1]; } diff --git a/src/accessor/grib_accessor_class_bits.cc b/src/accessor/grib_accessor_class_bits.cc index 54ad9d0e2..da1c05205 100644 --- a/src/accessor/grib_accessor_class_bits.cc +++ b/src/accessor/grib_accessor_class_bits.cc @@ -9,6 +9,8 @@ */ #include "grib_accessor_class_bits.h" +#include "NumericLimits.h" + grib_accessor_bits_t _grib_accessor_bits{}; grib_accessor* grib_accessor_bits = &_grib_accessor_bits; @@ -160,7 +162,7 @@ int grib_accessor_bits_t::pack_long(const long* val, size_t* len) } #endif - maxval = (1 << length) - 1; + maxval = NumericLimits::max(length); if (*val > maxval) { grib_context_log(h->context, GRIB_LOG_ERROR, "key=%s: Trying to encode value of %ld but the maximum allowable value is %ld (number of bits=%ld)", diff --git a/src/accessor/grib_accessor_class_bufr_data_array.cc b/src/accessor/grib_accessor_class_bufr_data_array.cc index b5884640a..06d5266a6 100644 --- a/src/accessor/grib_accessor_class_bufr_data_array.cc +++ b/src/accessor/grib_accessor_class_bufr_data_array.cc @@ -13,6 +13,7 @@ #include "grib_accessor_class_expanded_descriptors.h" #include "grib_accessor_class_bufr_data_element.h" #include "grib_accessor_class_variable.h" +#include "NumericLimits.h" grib_accessor_bufr_data_array_t _grib_accessor_bufr_data_array{}; grib_accessor* grib_accessor_bufr_data_array = &_grib_accessor_bufr_data_array; @@ -131,8 +132,8 @@ int grib_accessor_bufr_data_array_t::tableB_override_set_key(grib_handle* h) /* Check numBits is sufficient for entries in the overridden reference values list*/ static int check_overridden_reference_values(const grib_context* c, long* refValList, size_t refValListSize, int numBits) { - const long maxval = (1 << (numBits - 1)) - 1; - const long minval = -(1 << (numBits - 1)); + const long maxval = NumericLimits::max(numBits); + const long minval = NumericLimits::min(numBits); size_t i = 0; for (i = 0; i < refValListSize; ++i) { grib_context_log(c, GRIB_LOG_DEBUG, "check_overridden_reference_values: refValList[%ld]=%ld", i, refValList[i]); diff --git a/src/accessor/grib_accessor_class_from_scale_factor_scaled_value.cc b/src/accessor/grib_accessor_class_from_scale_factor_scaled_value.cc index e8471e32f..771186c1a 100644 --- a/src/accessor/grib_accessor_class_from_scale_factor_scaled_value.cc +++ b/src/accessor/grib_accessor_class_from_scale_factor_scaled_value.cc @@ -10,6 +10,7 @@ */ #include "grib_accessor_class_from_scale_factor_scaled_value.h" +#include "NumericLimits.h" grib_accessor_from_scale_factor_scaled_value_t _grib_accessor_from_scale_factor_scaled_value{}; grib_accessor* grib_accessor_from_scale_factor_scaled_value = &_grib_accessor_from_scale_factor_scaled_value; @@ -64,8 +65,8 @@ int grib_accessor_from_scale_factor_scaled_value_t::pack_double(const double* va } value_accessor_num_bits = value_accessor->length_ * 8; factor_accessor_num_bits = factor_accessor->length_ * 8; - maxval_value = (1UL << value_accessor_num_bits) - 2; // exclude missing - maxval_factor = (1UL << factor_accessor_num_bits) - 2; // exclude missing + maxval_value = NumericLimits::max(value_accessor_num_bits); // exclude missing + maxval_factor = NumericLimits::max(factor_accessor_num_bits); // exclude missing if (strcmp(factor_accessor->class_name_, "signed") == 0) { maxval_factor = (1UL << (factor_accessor_num_bits - 1)) - 1; } diff --git a/src/accessor/grib_accessor_class_unsigned.cc b/src/accessor/grib_accessor_class_unsigned.cc index 95f802150..e4bd0b958 100644 --- a/src/accessor/grib_accessor_class_unsigned.cc +++ b/src/accessor/grib_accessor_class_unsigned.cc @@ -9,6 +9,7 @@ */ #include "grib_accessor_class_unsigned.h" +#include "NumericLimits.h" grib_accessor_unsigned_t _grib_accessor_unsigned{}; grib_accessor* grib_accessor_unsigned = &_grib_accessor_unsigned; @@ -116,7 +117,7 @@ int grib_accessor_unsigned_t::pack_long_unsigned_helper(const long* val, size_t* if (!value_is_missing(v)) { const long nbits = nbytes_ * 8; if (nbits < 33) { - unsigned long maxval = (1UL << nbits) - 1; + unsigned long maxval = NumericLimits::max(nbits); if (maxval > 0 && v > maxval) { /* See ECC-1002 */ grib_context_log(context_, GRIB_LOG_ERROR, "Key \"%s\": Trying to encode value of %ld but the maximum allowable value is %lu (number of bits=%ld)", diff --git a/tests/grib_set_fail.sh b/tests/grib_set_fail.sh index e3fd7acd9..2ced80e06 100755 --- a/tests/grib_set_fail.sh +++ b/tests/grib_set_fail.sh @@ -77,7 +77,7 @@ if [ $ECCODES_ON_WINDOWS -eq 0 ]; then status=$? set -e [ $status -ne 0 ] - grep -q "Trying to encode value of 2147483648 but the allowable range is -2147483647 to 2147483647" $temp + grep -q "Trying to encode value of 2147483648 but the allowable range is -2147483648 to 2147483647" $temp set +e ${tools_dir}/grib_set -s forecastTime=-2147483650 $input $outfile 2>$temp