Merge pull request #95 from ecmwf/bugfix/89-segmentation-fault-caused…

…-by-buffer-overflow-in-channelbuffer

Add defensive pointer boundary checks to log targets to avoid SEGFAUL…

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() {
@@ -68,10 +66,13 @@ 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 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_) {
-        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 +121,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() {

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);
 }
 

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_);

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<char*>(start), const_cast<char*>(end));
     target_->write(start, end);
 }

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() {

src/eckit/log/RotationTarget.cc

@@ -44,6 +44,7 @@ class RotationOutputStream {
     }
 
     void write(const char* start, const char* end) {
+        if(start >= end) return;
         AutoLock<StaticMutex> 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() {

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);
     }

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;