From fdaa5e13927e646d4fd23bf170f174f3cebb6bd1 Mon Sep 17 00:00:00 2001 From: Danny Robson Date: Thu, 24 Sep 2020 08:03:41 +1000 Subject: [PATCH] assert: split CHECK_LIMIT into INCLUSIVE and INDEX LIMIT hid an off-by-one bug when tests used end iterators. We rename the assertion to uncover all uses of the flawed implementation, and split it into an identical assertion, and one intended to protect against iterator ends. --- array/darray.hpp | 6 ++++-- array/parray.cpp | 4 ++-- array/varray.hpp | 4 ++-- bezier2.cpp | 4 ++-- bezier3.cpp | 2 +- buffer/circular.cpp | 6 +++--- coord/ops.hpp | 2 +- debug/assert.hpp | 26 ++++++++++++++++++++++++-- matrix.cpp | 2 +- pool.hpp | 7 ++----- region.cpp | 2 +- uri.hpp | 2 +- view.hpp | 11 +++++++---- 13 files changed, 51 insertions(+), 27 deletions(-) diff --git a/array/darray.hpp b/array/darray.hpp index a32a0037..2a658093 100644 --- a/array/darray.hpp +++ b/array/darray.hpp @@ -127,7 +127,8 @@ namespace cruft { void erase (iterator pos) { - CHECK_LIMIT (pos, begin (), end ()); + CHECK_GE (pos, begin ()); + CHECK_LT (pos, end ()); for (auto cursor = pos + 1; cursor != end (); ++cursor) *(cursor - 1) = std::move (*cursor); @@ -147,7 +148,8 @@ namespace cruft { iterator insert (const_iterator pos, size_type count, ValueT const &val) { // Ensure we have enough space - CHECK_LIMIT (pos, begin (), end ()); + CHECK_GE (pos, begin ()); + CHECK_LT (pos, end ()); CHECK_LE (m_size, CapacityV - count); std::move_backward (pos, cend (), end () + count); diff --git a/array/parray.cpp b/array/parray.cpp index 79796702..63c085a7 100644 --- a/array/parray.cpp +++ b/array/parray.cpp @@ -29,7 +29,7 @@ template DataT& parray::operator[] (SizeT idx) { - CHECK_LIMIT(idx, 0u, m_size); + CHECK_INDEX (idx, m_size); return data ()[idx]; } @@ -39,7 +39,7 @@ template const DataT& parray::operator[] (SizeT idx) const { - CHECK_LIMIT(idx, 0u, m_size); + CHECK_INDEX (idx, m_size); return data ()[idx]; } diff --git a/array/varray.hpp b/array/varray.hpp index 41b6283d..7c0df72b 100644 --- a/array/varray.hpp +++ b/array/varray.hpp @@ -59,8 +59,8 @@ namespace cruft { //--------------------------------------------------------------------- std::size_t size (void) const { return S; } - T& operator[] (std::size_t i) { CHECK_LIMIT (i, 0u, S); return m_data[i]; } - const T& operator[] (std::size_t i) const { CHECK_LIMIT (i, 0u, S); return m_data[i]; } + T& operator[] (std::size_t i) { CHECK_INDEX (i, S); return m_data[i]; } + const T& operator[] (std::size_t i) const { CHECK_INDEX (i, S); return m_data[i]; } auto data (void) { return m_data; } auto data (void) const { return m_data; } diff --git a/bezier2.cpp b/bezier2.cpp index 3991c0d6..4c8e9b49 100644 --- a/bezier2.cpp +++ b/bezier2.cpp @@ -18,7 +18,7 @@ namespace cruft { point2f bezier<2>::eval (float t) const { - CHECK_LIMIT (t, 0.f, 1.f); + CHECK_INCLUSIVE(t, 0.f, 1.f); const auto &P0 = m_coeffs[0]; const auto &P1 = m_coeffs[1]; @@ -56,7 +56,7 @@ namespace cruft { cruft::vector2f bezier<2>::d1 (const float t) const noexcept { - CHECK_LIMIT (t, 0.f, 1.f); + CHECK_INCLUSIVE (t, 0.f, 1.f); const auto &P0 = m_coeffs[0]; const auto &P1 = m_coeffs[1]; diff --git a/bezier3.cpp b/bezier3.cpp index 43769345..b2aa7b73 100644 --- a/bezier3.cpp +++ b/bezier3.cpp @@ -118,7 +118,7 @@ namespace cruft { cruft::vector2f bezier<3>::tangent (const float t) const { - CHECK_LIMIT (t, 0.f, 1.f); + CHECK_INCLUSIVE (t, 0.f, 1.f); return mix ( mix (m_coeffs[1] - m_coeffs[0], m_coeffs[2] - m_coeffs[1], t), diff --git a/buffer/circular.cpp b/buffer/circular.cpp index 13733b21..3840fa20 100644 --- a/buffer/circular.cpp +++ b/buffer/circular.cpp @@ -154,7 +154,7 @@ template typename circular::iterator circular::constrain (iterator cursor) { - CHECK_LIMIT (cursor, m_begin, m_begin + 2 * size ()); + CHECK_INCLUSIVE (cursor, m_begin, m_begin + 2 * size ()); return m_begin + (cursor - m_begin) % size (); } @@ -165,8 +165,8 @@ template cruft::view::iterator> circular::constrain (cruft::view window) { - CHECK_LIMIT (window.begin (), m_begin, m_begin + 2 * size ()); - CHECK_LIMIT (window.end (), m_begin, m_begin + 2 * size ()); + CHECK_INCLUSIVE (window.begin (), m_begin, m_begin + 2 * size ()); + CHECK_INCLUSIVE (window.end (), m_begin, m_begin + 2 * size ()); auto first = window.begin (); auto last = first + window.size (); diff --git a/coord/ops.hpp b/coord/ops.hpp index fa96b53c..7fc58823 100644 --- a/coord/ops.hpp +++ b/coord/ops.hpp @@ -1453,7 +1453,7 @@ namespace cruft { constexpr auto rshift (const K k, const int num, const K fill) { - CHECK_LIMIT (num, 0, int (K::elements)); + CHECK_INCLUSIVE (num, 0, int (K::elements)); K res {}; diff --git a/debug/assert.hpp b/debug/assert.hpp index 80a51bc3..c61cd9ae 100644 --- a/debug/assert.hpp +++ b/debug/assert.hpp @@ -236,14 +236,14 @@ constexpr bool debug_enabled = false; /////////////////////////////////////////////////////////////////////////////// -#define CHECK_LIMIT(V,L,H) do { \ +#define CHECK_INCLUSIVE(V,L,H) do { \ DEBUG_ONLY ( \ const auto &__v = (V); \ const auto &__l = (L); \ const auto &__h = (H); \ \ if (__v < __l || __v > __h) { \ - std::cerr << "expected limit\n" \ + std::cerr << "expected inclusive\n" \ "__l: " << #L << " is " << +__l << "\n" \ "__h: " << #H << " is " << +__h << "\n" \ "__v: " << #V << " is " << +__v << "\n"; \ @@ -252,6 +252,28 @@ constexpr bool debug_enabled = false; ); \ } while (0) + +/////////////////////////////////////////////////////////////////////////////// +#define CHECK_INDEX(V,H) do { \ + DEBUG_ONLY ( \ + const auto &__v = (V); \ + const auto &__h = (H); \ + \ + _Pragma("GCC diagnostic push") \ + _Pragma("GCC diagnostic ignored \"-Wtype-limits\"") \ + \ + if (intmax_t (__v) < 0 || __v >= __h) { \ + std::cerr << "expected index\n" \ + "__h: " << #H << " is " << +__h << "\n" \ + "__v: " << #V << " is " << +__v << "\n"; \ + breakpoint (); \ + }; \ + \ + _Pragma("GCC diagnostic pop") \ + ); \ +} while (0) + + /////////////////////////////////////////////////////////////////////////////// #define CHECK_NEQ(A,B) do { \ DEBUG_ONLY( \ diff --git a/matrix.cpp b/matrix.cpp index 6918a8ca..00ccbed8 100644 --- a/matrix.cpp +++ b/matrix.cpp @@ -166,7 +166,7 @@ template cruft::matrix4 cruft::perspective (T fov, T aspect, range Z) { - CHECK_LIMIT (fov, 0, 2 * cruft::pi); + CHECK_INCLUSIVE (fov, 0, 2 * cruft::pi); CHECK_GE (Z.lo, 0); CHECK_GE (Z.hi, 0); diff --git a/pool.hpp b/pool.hpp index fc38667e..a9f12e22 100644 --- a/pool.hpp +++ b/pool.hpp @@ -225,11 +225,8 @@ namespace cruft { /////////////////////////////////////////////////////////////////////// size_t index (T const *ptr) const { - CHECK_LIMIT ( - ptr, - reinterpret_cast (m_store), - reinterpret_cast (m_store) + capacity () - ); + CHECK_GE (ptr, reinterpret_cast (m_store)); + CHECK_LT (ptr, reinterpret_cast (m_store) + capacity ()); return ptr - reinterpret_cast (m_store); } diff --git a/region.cpp b/region.cpp index d459d117..f40e1a22 100644 --- a/region.cpp +++ b/region.cpp @@ -248,7 +248,7 @@ template cruft::region<2,T> cruft::rotate90 (cruft::region<2,T> obj, int steps) { - CHECK_LIMIT (steps, 0, 3); + CHECK_INCLUSIVE (steps, 0, 3); switch (steps) { case 0: diff --git a/uri.hpp b/uri.hpp index 8503f1ee..88777ac0 100644 --- a/uri.hpp +++ b/uri.hpp @@ -69,7 +69,7 @@ namespace cruft { view get (component c) const& { - CHECK_LIMIT (c, 0, NUM_COMPONENTS); + CHECK_INDEX (c, NUM_COMPONENTS); return m_views[c]; } diff --git a/view.hpp b/view.hpp index ff56c8f1..1b719778 100644 --- a/view.hpp +++ b/view.hpp @@ -364,8 +364,8 @@ namespace cruft { auto slice (IndexA a, IndexB b) const { - CHECK_LIMIT (cruft::abs (a), IndexA {0}, cruft::cast::lossless (size ())); - CHECK_LIMIT (cruft::abs (b), IndexB {0}, cruft::cast::lossless (size ())); + CHECK_INCLUSIVE (cruft::abs (a), IndexA {0}, cruft::cast::lossless (size ())); + CHECK_INCLUSIVE (cruft::abs (b), IndexB {0}, cruft::cast::lossless (size ())); auto first = m_begin; auto last = m_begin; @@ -485,7 +485,9 @@ namespace cruft { constexpr auto&& operator[] (size_t idx) noexcept { - CHECK_LIMIT (idx, 0u, size ()); + CHECK_GE (idx, 0u); + CHECK_LT (idx, size ()); + return *std::next (begin (), idx); } @@ -493,7 +495,8 @@ namespace cruft { constexpr auto&& operator[] (size_t idx) const noexcept { - CHECK_LIMIT (idx, 0u, size ()); + CHECK_GE (idx, 0u); + CHECK_LT (idx, size ()); return *std::next (begin (), idx); }