From d7d610efc368aaa7cbb4b6742c0ef01c5b994fc3 Mon Sep 17 00:00:00 2001 From: Philipp Geier Date: Thu, 23 Nov 2023 16:26:09 +0100 Subject: [PATCH 1/4] Add defensive pointer boundary checks to log targets to avoid SEGFAULTS with raceconditions --- src/eckit/log/ChannelBuffer.cc | 14 ++++++-------- src/eckit/log/FileTarget.cc | 1 + src/eckit/log/LineBasedTarget.cc | 3 ++- src/eckit/log/MonitorTarget.cc | 1 + src/eckit/log/OStreamTarget.cc | 1 + src/eckit/log/RotationTarget.cc | 2 ++ src/eckit/log/TeeTarget.cc | 5 +++-- src/eckit/log/WrapperTarget.cc | 4 ++-- 8 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/eckit/log/ChannelBuffer.cc b/src/eckit/log/ChannelBuffer.cc index ad1fe2f0d..2b1432f7a 100644 --- a/src/eckit/log/ChannelBuffer.cc +++ b/src/eckit/log/ChannelBuffer.cc @@ -23,11 +23,9 @@ namespace eckit { //---------------------------------------------------------------------------------------------------------------------- -ChannelBuffer::ChannelBuffer(std::size_t size) : - std::streambuf(), target_(0), buffer_(size) { +ChannelBuffer::ChannelBuffer(std::size_t size) : std::streambuf(), target_(0), buffer_(size) { ASSERT(size); - char* base = &buffer_.front(); - setp(base, base + buffer_.size()); + setp(buffer_.data(), buffer_.data() + buffer_.size()); } ChannelBuffer::~ChannelBuffer() { @@ -69,9 +67,10 @@ void ChannelBuffer::reset() { bool ChannelBuffer::dumpBuffer() { if (target_) { - target_->write(pbase(), pptr()); + // Explicitly check that `pptr()` is not larger than end of buffer. Racecondition can end up adding larger values. + target_->write(buffer_.data(), std::min(pptr(), buffer_.data() + buffer_.size())); } - setp(pbase(), epptr()); + setp(buffer_.data(), buffer_.data() + buffer_.size()); return true; } @@ -120,8 +119,7 @@ std::streambuf::int_type ChannelBuffer::overflow(std::streambuf::int_type ch) { return sync(); } dumpBuffer(); - sputc(ch); - return traits_type::to_int_type(ch); + return sputc(ch); } std::streambuf::int_type ChannelBuffer::sync() { diff --git a/src/eckit/log/FileTarget.cc b/src/eckit/log/FileTarget.cc index dfdd00e7b..c29fe86a5 100644 --- a/src/eckit/log/FileTarget.cc +++ b/src/eckit/log/FileTarget.cc @@ -39,6 +39,7 @@ FileTarget::~FileTarget() { } void FileTarget::write(const char* start, const char* end) { + if (start >= end) return; out_.write(start, end - start); } diff --git a/src/eckit/log/LineBasedTarget.cc b/src/eckit/log/LineBasedTarget.cc index 6e9d0238d..0ed1f0807 100644 --- a/src/eckit/log/LineBasedTarget.cc +++ b/src/eckit/log/LineBasedTarget.cc @@ -36,10 +36,11 @@ void LineBasedTarget::reserve(size_t size) { } void LineBasedTarget::write(const char* start, const char* end) { + if(start >= end) return; reserve(position_ + (end - start) + 1); - while (start != end) { + while (start < end) { if (*start == '\n') { buffer_[position_] = 0; line(buffer_); diff --git a/src/eckit/log/MonitorTarget.cc b/src/eckit/log/MonitorTarget.cc index 6ffe154d9..2dc0bbaa8 100644 --- a/src/eckit/log/MonitorTarget.cc +++ b/src/eckit/log/MonitorTarget.cc @@ -24,6 +24,7 @@ MonitorTarget::MonitorTarget(LogTarget* target) : MonitorTarget::~MonitorTarget() {} void MonitorTarget::write(const char* start, const char* end) { + if (start >= end) return; Monitor::instance().out(const_cast(start), const_cast(end)); target_->write(start, end); } diff --git a/src/eckit/log/OStreamTarget.cc b/src/eckit/log/OStreamTarget.cc index 7c12eab6a..c58e69dee 100644 --- a/src/eckit/log/OStreamTarget.cc +++ b/src/eckit/log/OStreamTarget.cc @@ -22,6 +22,7 @@ OStreamTarget::OStreamTarget(std::ostream& out) : OStreamTarget::~OStreamTarget() {} void OStreamTarget::write(const char* start, const char* end) { + if (start >= end) return; out_.write(start, end - start); } void OStreamTarget::flush() { diff --git a/src/eckit/log/RotationTarget.cc b/src/eckit/log/RotationTarget.cc index 9b02cf2bc..9219e0c61 100644 --- a/src/eckit/log/RotationTarget.cc +++ b/src/eckit/log/RotationTarget.cc @@ -44,6 +44,7 @@ class RotationOutputStream { } void write(const char* start, const char* end) { + if(start >= end) return; AutoLock lock(local_mutex); rotout().write(start, end - start); } @@ -125,6 +126,7 @@ RotationTarget::RotationTarget(const std::string& name) : RotationTarget::~RotationTarget() {} void RotationTarget::write(const char* start, const char* end) { + if(start >= end) return; RotationOutputStream::instance(name_).write(start, end); } void RotationTarget::flush() { diff --git a/src/eckit/log/TeeTarget.cc b/src/eckit/log/TeeTarget.cc index 6d4a04fde..ff0edf208 100644 --- a/src/eckit/log/TeeTarget.cc +++ b/src/eckit/log/TeeTarget.cc @@ -17,8 +17,7 @@ namespace eckit { //---------------------------------------------------------------------------------------------------------------------- -TeeTarget::TeeTarget(LogTarget* left, LogTarget* right) : - left_(left), right_(right) { +TeeTarget::TeeTarget(LogTarget* left, LogTarget* right) : left_(left), right_(right) { if (left_) { left_->attach(); @@ -40,6 +39,8 @@ TeeTarget::~TeeTarget() { } void TeeTarget::write(const char* start, const char* end) { + if (start >= end) + return; if (left_) { left_->write(start, end); } diff --git a/src/eckit/log/WrapperTarget.cc b/src/eckit/log/WrapperTarget.cc index 9f186ff50..59485b6ff 100644 --- a/src/eckit/log/WrapperTarget.cc +++ b/src/eckit/log/WrapperTarget.cc @@ -35,7 +35,7 @@ void WrapperTarget::write(const char* start, const char* end) { const char* begin = start; - while (start != end) { + while (start < end) { if (*start == '\n') { target_->write(begin, start); writeSuffix(); @@ -52,7 +52,7 @@ void WrapperTarget::write(const char* start, const char* end) { start++; } - if (begin != end) { + if (begin < end) { if (prefix_) { writePrefix(); prefix_ = false; From 5bd4c57d9f498fb14781c8c9351542b8dfe96e27 Mon Sep 17 00:00:00 2001 From: Philipp Geier Date: Wed, 6 Dec 2023 12:34:45 +0100 Subject: [PATCH 2/4] Update ChannelBuffer.cc Add comment.... --- src/eckit/log/ChannelBuffer.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/eckit/log/ChannelBuffer.cc b/src/eckit/log/ChannelBuffer.cc index 2b1432f7a..6f58523fa 100644 --- a/src/eckit/log/ChannelBuffer.cc +++ b/src/eckit/log/ChannelBuffer.cc @@ -66,6 +66,7 @@ void ChannelBuffer::reset() { } bool ChannelBuffer::dumpBuffer() { + // When setting and using pointers we should have boundary checks to avoid race conditions. See https://github.com/ecmwf/eckit/issues/89 if (target_) { // Explicitly check that `pptr()` is not larger than end of buffer. Racecondition can end up adding larger values. target_->write(buffer_.data(), std::min(pptr(), buffer_.data() + buffer_.size())); From 202318f84db74e1b9fa1534514d6eee8de7b3b59 Mon Sep 17 00:00:00 2001 From: Philipp Geier Date: Wed, 6 Dec 2023 12:37:33 +0100 Subject: [PATCH 3/4] Update ChannelBuffer.cc --- src/eckit/log/ChannelBuffer.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/eckit/log/ChannelBuffer.cc b/src/eckit/log/ChannelBuffer.cc index 6f58523fa..852ca31d5 100644 --- a/src/eckit/log/ChannelBuffer.cc +++ b/src/eckit/log/ChannelBuffer.cc @@ -66,7 +66,8 @@ void ChannelBuffer::reset() { } bool ChannelBuffer::dumpBuffer() { - // When setting and using pointers we should have boundary checks to avoid race conditions. See https://github.com/ecmwf/eckit/issues/89 + // When setting and using pointers we should have boundary checks. In a multi threaded environment we already have experienced weird behaviour. + // With thiese checks the race conditions are not gone but they won't cause any segfaults. See https://github.com/ecmwf/eckit/issues/89 if (target_) { // Explicitly check that `pptr()` is not larger than end of buffer. Racecondition can end up adding larger values. target_->write(buffer_.data(), std::min(pptr(), buffer_.data() + buffer_.size())); From 6d3518ac9eea73b5021ab8a4f8bc23eeae600080 Mon Sep 17 00:00:00 2001 From: Philipp Geier Date: Wed, 6 Dec 2023 12:38:25 +0100 Subject: [PATCH 4/4] Update ChannelBuffer.cc --- src/eckit/log/ChannelBuffer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eckit/log/ChannelBuffer.cc b/src/eckit/log/ChannelBuffer.cc index 852ca31d5..d5b20cef4 100644 --- a/src/eckit/log/ChannelBuffer.cc +++ b/src/eckit/log/ChannelBuffer.cc @@ -67,7 +67,7 @@ void ChannelBuffer::reset() { bool ChannelBuffer::dumpBuffer() { // When setting and using pointers we should have boundary checks. In a multi threaded environment we already have experienced weird behaviour. - // With thiese checks the race conditions are not gone but they won't cause any segfaults. See https://github.com/ecmwf/eckit/issues/89 + // With these checks the race conditions are not gone but they won't cause any segfaults. See https://github.com/ecmwf/eckit/issues/89 if (target_) { // Explicitly check that `pptr()` is not larger than end of buffer. Racecondition can end up adding larger values. target_->write(buffer_.data(), std::min(pptr(), buffer_.data() + buffer_.size()));