From 6098d523354091db0eb7cb2745cc09b20fc5b80f Mon Sep 17 00:00:00 2001 From: "Jonathan R. Madsen" Date: Thu, 12 Sep 2024 18:25:05 -0500 Subject: [PATCH] Prevent misaligned read from common::container::ring_buffer (#1076) - causes undefined behavior --- source/lib/common/container/ring_buffer.cpp | 12 +++++++---- source/lib/common/container/ring_buffer.hpp | 24 ++++++++++++++------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/source/lib/common/container/ring_buffer.cpp b/source/lib/common/container/ring_buffer.cpp index 0f166e445d..f98829589b 100644 --- a/source/lib/common/container/ring_buffer.cpp +++ b/source/lib/common/container/ring_buffer.cpp @@ -138,7 +138,7 @@ ring_buffer::request(size_t _length, size_t _align, bool _wrap) { if(m_ptr == nullptr || m_size == 0) return nullptr; - if(is_full()) return (_wrap) ? retrieve(_length) : nullptr; + if(is_full()) return (_wrap) ? retrieve(_length, _align) : nullptr; LOG_IF(FATAL, _align == 0) << "alignment must be non-zero"; @@ -169,7 +169,7 @@ ring_buffer::request(size_t _length, size_t _align, bool _wrap) // void* -ring_buffer::retrieve(size_t _length) const +ring_buffer::retrieve(size_t _length, size_t _align) const { if(m_ptr == nullptr || m_size == 0) return nullptr; @@ -179,6 +179,7 @@ ring_buffer::retrieve(size_t _length) const // if read count is at the tail of buffer, bump to the end of buffer size_t _read_count = 0; size_t _offset = 0; + size_t _read_pos = 0; do { if(_length > count()) return nullptr; @@ -186,11 +187,14 @@ ring_buffer::retrieve(size_t _length) const _read_count = m_read_count.load(std::memory_order_acquire); auto _modulo = m_size - (_read_count % m_size); if(_modulo < _length) _offset = _modulo; + auto _align_modulo = (_read_count % _align); + auto _align_offset = (_align_modulo > 0) ? (_align - _align_modulo) : 0; + _read_pos = _read_count + _align_offset; } while(!m_read_count.compare_exchange_strong( - _read_count, _read_count + _length + _offset, std::memory_order_seq_cst)); + _read_count, _read_pos + _length + _offset, std::memory_order_seq_cst)); // pointer in buffer - void* _out = read_ptr(_read_count); + void* _out = read_ptr(_read_pos); return _out; } diff --git a/source/lib/common/container/ring_buffer.hpp b/source/lib/common/container/ring_buffer.hpp index 4501c8364e..ebc5ac6bd9 100644 --- a/source/lib/common/container/ring_buffer.hpp +++ b/source/lib/common/container/ring_buffer.hpp @@ -74,11 +74,17 @@ struct ring_buffer /// Destroy ring buffer. void destroy(); - /// Request a pointer for writing at least \param n bytes. + /// Request a pointer for writing at least \param n bytes. If the current write pointer is not + /// perfectly divisible by \param align (i.e. if write_addr % align != 0), the returned address + /// will be shifted to an address that is a multiple of that alignment to prevent undefined + /// behavior. void* request(size_t n, size_t align, bool wrap = true); - /// Retrieve a pointer for reading at least \param n bytes. - void* retrieve(size_t n) const; + /// Retrieve a pointer for reading at least \param n bytes. If the current read pointer is not + /// perfectly divisible by \param align (i.e. if read_addr % align != 0), the returned address + /// will be shifted to an address that is a multiple of that alignment to prevent undefined + /// behavior. + void* retrieve(size_t n, size_t align) const; /// Write class-type data to buffer (uses placement new). template @@ -225,8 +231,9 @@ ring_buffer::read(Tp* _dest, std::enable_if_t::value, int>) co { if(is_empty() || _dest == nullptr) return {0, nullptr}; - auto _length = sizeof(Tp); - void* _out_p = retrieve(_length); + constexpr auto _length = sizeof(Tp); + constexpr auto _align = alignof(Tp); + void* _out_p = retrieve(_length, _align); if(_out_p == nullptr) return {0, nullptr}; @@ -245,8 +252,9 @@ ring_buffer::read(Tp* _dest, std::enable_if_t::value, int>) c { if(is_empty() || _dest == nullptr) return {0, nullptr}; - auto _length = sizeof(Tp); - void* _out_p = retrieve(_length); + constexpr auto _length = sizeof(Tp); + constexpr auto _align = alignof(Tp); + void* _out_p = retrieve(_length, _align); if(_out_p == nullptr) return {0, nullptr}; @@ -268,7 +276,7 @@ ring_buffer::retrieve() const { if(m_ptr == nullptr) return nullptr; - return reinterpret_cast(retrieve(sizeof(Tp))); + return reinterpret_cast(retrieve(sizeof(Tp), alignof(Tp))); } // } // namespace base