From: Sage Weil <sage@redhat.com>
Date: Sat, 4 Aug 2018 03:16:21 +0000 (-0500)
Subject: log/Log: fix buffer overflow
X-Git-Tag: v14.0.1~637^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F23422%2Fhead;p=ceph.git

log/Log: fix buffer overflow

If we have a long line that has to be dynamically allocated, we need to
adjust m_log_buf_pos accordingly afterward.  In particular,
_write_and_copy does this adjustment, which means the later += line_used
is not only unnecessary but very bad, since line_used is > MAX_LOG_BUF.

Simplify all of this by getting rid of _write_and_copy: if we are writing
a big chunk let's just send the whole thing to m_fd and not bother keeping
the tail of it around for later.

Fix _log_safe_write to write the passed buffer, not m_log_buf.

Replace the paranoid check with a simple assert.

Fixes: 65da5ba216cafb8a91893d0e7fc092209f0fb286
Fixes: https://github.com/ceph/ceph/pull/23422
Signed-off-by: Sage Weil <sage@redhat.com>
---

diff --git a/src/log/Log.cc b/src/log/Log.cc
index ee17ebacad99..6645a4814f10 100644
--- a/src/log/Log.cc
+++ b/src/log/Log.cc
@@ -304,7 +304,7 @@ void Log::_log_safe_write(const char* what, size_t write_len)
 {
   if (m_fd < 0)
     return;
-  int r = safe_write(m_fd, m_log_buf, m_log_buf_pos);
+  int r = safe_write(m_fd, what, write_len);
   if (r != m_fd_last_error) {
     if (r < 0)
       cerr << "problem writing to " << m_log_file
@@ -322,23 +322,6 @@ void Log::_flush_logbuf()
   }
 }
 
-// write part of "what" directly to disk, copy remaining part to m_log_buf
-// for later coalescing
-void Log::_write_and_copy(char* what, size_t len)
-{
-  size_t write_len = len - (len & (MAX_LOG_BUF - 1));
-  if (write_len) {
-    _log_safe_write(what, write_len);
-    what += write_len;
-  }
-
-  write_len = len - write_len;
-  if (write_len) {
-     maybe_inline_memcpy((void*)m_log_buf, (void*)what, write_len, 32);
-     m_log_buf_pos = write_len;
-  }
-}
-
 void Log::_flush(EntryQueue *t, EntryQueue *requeue, bool crash)
 {
   Entry *e = nullptr;
@@ -385,7 +368,7 @@ void Log::_flush(EntryQueue *t, EntryQueue *requeue, bool crash)
       // this flushes the existing buffers if either line is longer
       // than our buffer, or buffer is too full to fit it
       if (m_log_buf_pos + line_size >= MAX_LOG_BUF) {
-          _flush_logbuf();
+	_flush_logbuf();
       }
       if (need_dynamic) {
         line = new char[line_size];
@@ -401,11 +384,7 @@ void Log::_flush(EntryQueue *t, EntryQueue *requeue, bool crash)
 			(unsigned long)e->m_thread, e->m_prio);
 
       line_used += e->snprintf(line + line_used, line_size - line_used - 1);
-      if (line_used > line_size - 1) { //paranoid check, buf was declared
-				   //to hold everything
-        line_used = line_size - 1;
-        line[line_used] = 0;
-      }
+      assert(line_used < line_size - 1);
 
       if (do_syslog) {
         syslog(LOG_USER|LOG_INFO, "%s", line);
@@ -416,16 +395,20 @@ void Log::_flush(EntryQueue *t, EntryQueue *requeue, bool crash)
       }
 
       if (do_fd) {
-        line[line_used] = '\n';
-        if (need_dynamic) {
-          _write_and_copy(line, line_used + 1);
-        }
+	line[line_used] = '\n';
+	if (need_dynamic) {
+	  _log_safe_write(line, line_used + 1);
+	  m_log_buf_pos = 0;
+	} else {
+	  m_log_buf_pos += line_used + 1;
+	}
+      } else {
+	m_log_buf_pos = 0;
       }
 
-      if (need_dynamic)
+      if (need_dynamic) {
         delete[] line;
-
-      m_log_buf_pos += line_used + 1;
+      }
     }
 
     if (do_graylog2 && m_graylog) {
diff --git a/src/log/Log.h b/src/log/Log.h
index e79b7815bb18..07505e6de258 100644
--- a/src/log/Log.h
+++ b/src/log/Log.h
@@ -62,7 +62,6 @@ class Log : private Thread
   void *entry() override;
 
   void _log_safe_write(const char* what, size_t write_len);
-  void _write_and_copy(char* what, size_t len);
   void _flush_logbuf();
   void _flush(EntryQueue *q, EntryQueue *requeue, bool crash);