From e66ea2888b84cbc3167ee5fff9ed1de2e236a6c2 Mon Sep 17 00:00:00 2001 From: Devin Date: Wed, 23 Sep 2026 09:56:59 +0200 Subject: [PATCH 1/5] Bound packed channel reference copies to the channel's byte range get_data()/set_data() copied sizeof(BitField) bytes unconditionally. For bit-aligned images whose last pixel occupies fewer bits than a full BitField (e.g. bit_aligned_image3_type<1,2,1> = 4 bits/px on a 16-bit field), accessing the final pixel read or wrote past the end of the image buffer (heap-buffer-overflow, #808). Copy only ceil((first_bit + NumBits) / 8) bytes -- the bytes that actually contain the referenced channel's bits. Untouched bits keep their storage contents since the read-modify-write in set_unsafe() writes back exactly the byte range it read. --- include/boost/gil/channel.hpp | 44 ++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/include/boost/gil/channel.hpp b/include/boost/gil/channel.hpp index 9e81c5790f..4ea2082dab 100644 --- a/include/boost/gil/channel.hpp +++ b/include/boost/gil/channel.hpp @@ -452,6 +452,32 @@ class packed_channel_reference_base } #endif + // Bounded variants that copy only the bytes containing the referenced + // channel's bits. The last bit field of a bit-aligned image row may + // occupy fewer than sizeof(bitfield_t) bytes, so a full-width copy + // would read or write past the end of the image buffer. + auto get_data(std::size_t byte_count) const -> bitfield_t + { + bitfield_t ret = 0; + unsigned char const* src = gil_reinterpret_cast_c(_data_ptr); + unsigned char* dst = gil_reinterpret_cast(&ret); + if (byte_count > sizeof(bitfield_t)) + byte_count = sizeof(bitfield_t); + for (std::size_t i = 0; i < byte_count; ++i) + dst[i] = src[i]; + return ret; + } + + void set_data(bitfield_t const& val, std::size_t byte_count) const + { + unsigned char const* src = gil_reinterpret_cast_c(&val); + unsigned char* dst = gil_reinterpret_cast(_data_ptr); + if (byte_count > sizeof(bitfield_t)) + byte_count = sizeof(bitfield_t); + for (std::size_t i = 0; i < byte_count; ++i) + dst[i] = src[i]; + } + private: void set(integer_t value) const { // can this be done faster?? this->derived().set_unsafe(((value % num_values) + num_values) % num_values); @@ -526,7 +552,7 @@ class packed_channel_reference auto first_bit() const -> unsigned int { return FirstBit; } - auto get() const -> integer_t { return integer_t((this->get_data()&channel_mask) >> FirstBit); } + auto get() const -> integer_t { return integer_t((this->get_data((FirstBit + NumBits + 7) / 8)&channel_mask) >> FirstBit); } }; /// \ingroup PackedChannelReferenceModel @@ -555,19 +581,19 @@ class packed_channel_reference return *this; } - auto operator=(mutable_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data()); return *this; } - auto operator=(const_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data()); return *this; } + auto operator=(mutable_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data((FirstBit + NumBits + 7) / 8)); return *this; } + auto operator=(const_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data((FirstBit + NumBits + 7) / 8)); return *this; } template auto operator=(packed_dynamic_channel_reference const& ref) const -> packed_channel_reference const& { set_unsafe(ref.get()); return *this; } auto first_bit() const -> unsigned int { return FirstBit; } - auto get() const -> integer_t { return integer_t((this->get_data()&channel_mask) >> FirstBit); } - void set_unsafe(integer_t value) const { this->set_data((this->get_data() & ~channel_mask) | (( static_cast< BitField >( value )< integer_t { return integer_t((this->get_data((FirstBit + NumBits + 7) / 8)&channel_mask) >> FirstBit); } + void set_unsafe(integer_t value) const { this->set_data((this->get_data((FirstBit + NumBits + 7) / 8) & ~channel_mask) | (( static_cast< BitField >( value )<set_data((this->get_data() & ~channel_mask) | (other_bits & channel_mask)); } + void set_from_reference(const BitField& other_bits) const { this->set_data((this->get_data((FirstBit + NumBits + 7) / 8) & ~channel_mask) | (other_bits & channel_mask), (FirstBit + NumBits + 7) / 8); } }; }} // namespace boost::gil @@ -665,7 +691,7 @@ class packed_dynamic_channel_reference auto get() const -> integer_t { const BitField channel_mask = static_cast< integer_t >( parent_t::max_val ) <<_first_bit; - return static_cast< integer_t >(( this->get_data()&channel_mask ) >> _first_bit ); + return static_cast< integer_t >(( this->get_data((_first_bit + NumBits + 7) / 8)&channel_mask ) >> _first_bit ); } }; @@ -711,12 +737,12 @@ class packed_dynamic_channel_reference auto get() const -> integer_t { BitField const channel_mask = static_cast< integer_t >( parent_t::max_val ) << _first_bit; - return static_cast< integer_t >(( this->get_data()&channel_mask ) >> _first_bit ); + return static_cast< integer_t >(( this->get_data((_first_bit + NumBits + 7) / 8)&channel_mask ) >> _first_bit ); } void set_unsafe(integer_t value) const { const BitField channel_mask = static_cast< integer_t >( parent_t::max_val ) << _first_bit; - this->set_data((this->get_data() & ~channel_mask) | value<<_first_bit); + this->set_data((this->get_data((_first_bit + NumBits + 7) / 8) & ~channel_mask) | value<<_first_bit, (_first_bit + NumBits + 7) / 8); } }; } } // namespace boost::gil From f55e525b668bfa72a9df0516d171247928df049e Mon Sep 17 00:00:00 2001 From: Samuel Debionne Date: Tue, 13 Oct 2026 10:05:46 +0200 Subject: [PATCH 2/5] Factorize computation of channel bytes bound --- include/boost/gil/channel.hpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/include/boost/gil/channel.hpp b/include/boost/gil/channel.hpp index 4ea2082dab..8f37c6d0d2 100644 --- a/include/boost/gil/channel.hpp +++ b/include/boost/gil/channel.hpp @@ -539,6 +539,8 @@ class packed_channel_reference friend class packed_channel_reference; static const BitField channel_mask = static_cast(parent_t::max_val) << FirstBit; + // Number of bytes spanned by the channel + static constexpr std::size_t num_bytes = (FirstBit + NumBits + 7) / 8; void operator=(packed_channel_reference const&); public: @@ -552,7 +554,7 @@ class packed_channel_reference auto first_bit() const -> unsigned int { return FirstBit; } - auto get() const -> integer_t { return integer_t((this->get_data((FirstBit + NumBits + 7) / 8)&channel_mask) >> FirstBit); } + auto get() const -> integer_t { return integer_t((this->get_data(num_bytes)&channel_mask) >> FirstBit); } }; /// \ingroup PackedChannelReferenceModel @@ -565,6 +567,8 @@ class packed_channel_reference friend class packed_channel_reference; static const BitField channel_mask = static_cast< BitField >( parent_t::max_val ) << FirstBit; + // Number of bytes spanned by the channel + static constexpr std::size_t num_bytes = (FirstBit + NumBits + 7) / 8; public: using const_reference = packed_channel_reference const; @@ -581,19 +585,19 @@ class packed_channel_reference return *this; } - auto operator=(mutable_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data((FirstBit + NumBits + 7) / 8)); return *this; } - auto operator=(const_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data((FirstBit + NumBits + 7) / 8)); return *this; } + auto operator=(mutable_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data(num_bytes)); return *this; } + auto operator=(const_reference const& ref) const -> packed_channel_reference const& { set_from_reference(ref.get_data(num_bytes)); return *this; } template auto operator=(packed_dynamic_channel_reference const& ref) const -> packed_channel_reference const& { set_unsafe(ref.get()); return *this; } auto first_bit() const -> unsigned int { return FirstBit; } - auto get() const -> integer_t { return integer_t((this->get_data((FirstBit + NumBits + 7) / 8)&channel_mask) >> FirstBit); } - void set_unsafe(integer_t value) const { this->set_data((this->get_data((FirstBit + NumBits + 7) / 8) & ~channel_mask) | (( static_cast< BitField >( value )< integer_t { return integer_t((this->get_data(num_bytes)&channel_mask) >> FirstBit); } + void set_unsafe(integer_t value) const { this->set_data((this->get_data(num_bytes) & ~channel_mask) | (( static_cast< BitField >( value )<set_data((this->get_data((FirstBit + NumBits + 7) / 8) & ~channel_mask) | (other_bits & channel_mask), (FirstBit + NumBits + 7) / 8); } + void set_from_reference(const BitField& other_bits) const { this->set_data((this->get_data(num_bytes) & ~channel_mask) | (other_bits & channel_mask), num_bytes); } }; }} // namespace boost::gil From c504cc486756a1271d87b720bdd147a59ec9f69d Mon Sep 17 00:00:00 2001 From: Samuel Debionne Date: Tue, 13 Oct 2026 10:07:13 +0200 Subject: [PATCH 3/5] Add bit aligned channel bounds test --- test/core/image/CMakeLists.txt | 1 + test/core/image/Jamfile | 1 + test/core/image/bit_aligned_bounds.cpp | 103 +++++++++++++++++++++++++ 3 files changed, 105 insertions(+) create mode 100644 test/core/image/bit_aligned_bounds.cpp diff --git a/test/core/image/CMakeLists.txt b/test/core/image/CMakeLists.txt index 2f09041198..4d9a27b0c8 100644 --- a/test/core/image/CMakeLists.txt +++ b/test/core/image/CMakeLists.txt @@ -9,6 +9,7 @@ foreach(_name concepts alignment empty_dimensions + bit_aligned_bounds image) set(_test t_core_image_${_name}) set(_target test_core_image_${_name}) diff --git a/test/core/image/Jamfile b/test/core/image/Jamfile index c0cf0f4ad6..3077545ef6 100644 --- a/test/core/image/Jamfile +++ b/test/core/image/Jamfile @@ -12,4 +12,5 @@ compile concepts.cpp ; run alignment.cpp ; run empty_dimensions.cpp ; +run bit_aligned_bounds.cpp ; run image.cpp ; \ No newline at end of file diff --git a/test/core/image/bit_aligned_bounds.cpp b/test/core/image/bit_aligned_bounds.cpp new file mode 100644 index 0000000000..87e539ce30 --- /dev/null +++ b/test/core/image/bit_aligned_bounds.cpp @@ -0,0 +1,103 @@ +// +// Copyright 2026 Samuel Debionne +// +// Distributed under the Boost Software License, Version 1.0 +// See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt +// +#include + +#include + +#include +#include + +namespace gil = boost::gil; + +// Test cases of out-of-bounds access to the last pixel of bit-aligned images, +// see https://github.com/boostorg/gil/issues/808 +// A channel reference must only touch the bytes containing its bits, not the +// whole bit field. The main goal of these test cases is to trigger any memory +// error detectors (e.g. AddressSanitizer). + +// rgb121 pixels are 4 bits wide, stored in a 16-bit bit field +using rgb121_image_t = gil::bit_aligned_image3_type<1, 2, 1, gil::rgb_layout_t>::type; + +void test_write_read_every_pixel() +{ + for (std::ptrdiff_t w = 1; w <= 9; ++w) + { + for (std::ptrdiff_t h = 1; h <= 3; ++h) + { + rgb121_image_t image(w, h); + auto view = gil::view(image); + for (std::ptrdiff_t y = 0; y < h; ++y) + { + for (std::ptrdiff_t x = 0; x < w; ++x) + { + auto p = view(x, y); + gil::get_color(p, gil::red_t()) = (x + y) % 2; + gil::get_color(p, gil::green_t()) = (x + y) % 4; + gil::get_color(p, gil::blue_t()) = (x + y + 1) % 2; + } + } + for (std::ptrdiff_t y = 0; y < h; ++y) + { + for (std::ptrdiff_t x = 0; x < w; ++x) + { + auto p = view(x, y); + BOOST_TEST_EQ(int(gil::get_color(p, gil::red_t())), (x + y) % 2); + BOOST_TEST_EQ(int(gil::get_color(p, gil::green_t())), (x + y) % 4); + BOOST_TEST_EQ(int(gil::get_color(p, gil::blue_t())), (x + y + 1) % 2); + } + } + } + } +} + +void test_copy_pixels() +{ + for (std::ptrdiff_t w = 1; w <= 9; ++w) + { + rgb121_image_t src(w, 1); + auto view = gil::view(src); + for (std::ptrdiff_t x = 0; x < w; ++x) + { + gil::get_color(view(x, 0), gil::red_t()) = x % 2; + gil::get_color(view(x, 0), gil::green_t()) = x % 4; + gil::get_color(view(x, 0), gil::blue_t()) = (x + 1) % 2; + } + + rgb121_image_t dst(w, 1); + gil::copy_pixels(gil::const_view(src), gil::view(dst)); + BOOST_TEST(gil::equal_pixels(gil::const_view(src), gil::const_view(dst))); + } +} + +void test_packed_channel_reference_last_byte() +{ + // Channel occupying bits [5, 8) of a 16-bit bit field fits in the first byte + std::unique_ptr buffer(new unsigned char[1]{0}); + + gil::packed_channel_reference ref(buffer.get()); + ref = 5; + BOOST_TEST_EQ(int(ref), 5); + BOOST_TEST_EQ(int(buffer[0]), 5 << 5); + + gil::packed_channel_reference cref(buffer.get()); + BOOST_TEST_EQ(int(cref), 5); + + gil::packed_dynamic_channel_reference dref(buffer.get(), 5); + dref = 3; + BOOST_TEST_EQ(int(dref), 3); + BOOST_TEST_EQ(int(buffer[0]), 3 << 5); +} + +int main() +{ + test_write_read_every_pixel(); + test_copy_pixels(); + test_packed_channel_reference_last_byte(); + + return ::boost::report_errors(); +} From 80072dcbd8cd375a3590565122e37dde206d7e88 Mon Sep 17 00:00:00 2001 From: Samuel Debionne Date: Tue, 13 Oct 2026 10:14:56 +0200 Subject: [PATCH 4/5] Drop unbounded versions of channel get_data/set_data --- include/boost/gil/channel.hpp | 41 ++++------------------------------- 1 file changed, 4 insertions(+), 37 deletions(-) diff --git a/include/boost/gil/channel.hpp b/include/boost/gil/channel.hpp index 8f37c6d0d2..89f1755559 100644 --- a/include/boost/gil/channel.hpp +++ b/include/boost/gil/channel.hpp @@ -339,22 +339,6 @@ class packed_channel_value namespace detail { -template -struct static_copy_bytes -{ - void operator()(unsigned char const* from, unsigned char* to) const - { - *to = *from; - static_copy_bytes()(++from, ++to); - } -}; - -template <> -struct static_copy_bytes<0> -{ - void operator()(unsigned char const*, unsigned char*) const {} -}; - template class packed_channel_reference_base { @@ -435,27 +419,10 @@ class packed_channel_reference_base static const num_value_t num_values = static_cast< num_value_t >( 1 ) << NumBits ; static const max_value_t max_val = static_cast< max_value_t >( num_values - 1 ); -#if defined(BOOST_GIL_CONFIG_HAS_UNALIGNED_ACCESS) - const bitfield_t& get_data() const { return *static_cast(_data_ptr); } - void set_data(const bitfield_t& val) const { *static_cast< bitfield_t*>(_data_ptr) = val; } -#else - auto get_data() const -> bitfield_t - { - bitfield_t ret; - static_copy_bytes()(gil_reinterpret_cast_c(_data_ptr),gil_reinterpret_cast(&ret)); - return ret; - } - - void set_data(bitfield_t const& val) const - { - static_copy_bytes()(gil_reinterpret_cast_c(&val),gil_reinterpret_cast(_data_ptr)); - } -#endif - - // Bounded variants that copy only the bytes containing the referenced - // channel's bits. The last bit field of a bit-aligned image row may - // occupy fewer than sizeof(bitfield_t) bytes, so a full-width copy - // would read or write past the end of the image buffer. + // Copy only the bytes containing the referenced channel's bits. + // The last bit field of a bit-aligned image row may occupy fewer than + // sizeof(bitfield_t) bytes, so a full-width copy would read or write + // past the end of the image buffer (see issue #808). auto get_data(std::size_t byte_count) const -> bitfield_t { bitfield_t ret = 0; From ae379a979cf37425979a8d59c4ca10b244102fa7 Mon Sep 17 00:00:00 2001 From: Samuel Debionne Date: Tue, 13 Oct 2026 10:21:22 +0200 Subject: [PATCH 5/5] Fix and deprecate the DBOOST_GIL_CONFIG_HAS_UNALIGNED_ACCESS macro The build would previsouly fail with the macro set and the macro has no effect anymore --- include/boost/gil/channel.hpp | 29 +++++------------------------ 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/include/boost/gil/channel.hpp b/include/boost/gil/channel.hpp index 89f1755559..58292bf5a7 100644 --- a/include/boost/gil/channel.hpp +++ b/include/boost/gil/channel.hpp @@ -21,35 +21,16 @@ #ifdef BOOST_GIL_DOXYGEN_ONLY /// \def BOOST_GIL_CONFIG_HAS_UNALIGNED_ACCESS -/// \brief Define to allow unaligned memory access for models of packed channel value. -/// Theoretically (or historically?) on platforms which support dereferencing on -/// non-word memory boundary, unaligned access may result in performance improvement. -/// \warning Unfortunately, this optimization may be a C/C++ strict aliasing rules -/// violation, if accessed data buffer has effective type that cannot be aliased -/// without leading to undefined behaviour. +/// \brief Deprecated, has no effect. +/// Used to allow unaligned memory access for models of packed channel value. +/// Packed channel references now copy only the bytes spanned by the channel +/// (see issue #808), so this optimization no longer applies. #define BOOST_GIL_CONFIG_HAS_UNALIGNED_ACCESS #endif #ifdef BOOST_GIL_CONFIG_HAS_UNALIGNED_ACCESS -#if defined(sun) || defined(__sun) || \ // SunOS - defined(__osf__) || defined(__osf) || \ // Tru64 - defined(_hpux) || defined(hpux) || \ // HP-UX - defined(__arm__) || defined(__ARM_ARCH) || \ // ARM - defined(_AIX) // AIX -#error Unaligned access strictly disabled for some UNIX platforms or ARM architecture -#elif defined(__i386__) || defined(__x86_64__) || defined(__vax__) - // The check for little-endian architectures that tolerate unaligned memory - // accesses is just an optimization. Nothing will break if it fails to detect - // a suitable architecture. - // - // Unfortunately, this optimization may be a C/C++ strict aliasing rules violation - // if accessed data buffer has effective type that cannot be aliased - // without leading to undefined behaviour. -BOOST_PRAGMA_MESSAGE("CAUTION: Unaligned access tolerated on little-endian may cause undefined behaviour") -#else -#error Unaligned access disabled for unknown platforms and architectures +BOOST_PRAGMA_MESSAGE("BOOST_GIL_CONFIG_HAS_UNALIGNED_ACCESS is deprecated and has no effect") #endif -#endif // defined(BOOST_GIL_CONFIG_HAS_UNALIGNED_ACCESS) namespace boost { namespace gil {