[libvirt] [PATCH] logging: fix off-by-one bug
Valgrind caught that our log wrap-around was going 1 past the end. Regression introduced in commit b16f47a; previously the buffer was static and size+1 bytes, but now it is dynamic and exactly size bytes. * src/util/logging.c (virLogStr): Don't write past end of log. --- An alternative would be to malloc one larger; but since the log is likely to be a page size multiple and large enough to be worth malloc using mmap, going one larger is likely to waste the bulk of a page. Also, I like always NUL-terminating the current end of the log (without including that NUL in the log length). src/util/logging.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/logging.c b/src/util/logging.c index b972f8a..f4910ad 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -326,7 +326,7 @@ static void virLogStr(const char *str, int len) { return; if (len <= 0) len = strlen(str); - if (len > virLogSize) + if (len >= virLogSize) return; virLogLock(); @@ -336,13 +336,13 @@ static void virLogStr(const char *str, int len) { if (virLogEnd + len >= virLogSize) { tmp = virLogSize - virLogEnd; memcpy(&virLogBuffer[virLogEnd], str, tmp); - virLogBuffer[virLogSize] = 0; memcpy(&virLogBuffer[0], &str[tmp], len - tmp); virLogEnd = len - tmp; } else { memcpy(&virLogBuffer[virLogEnd], str, len); virLogEnd += len; } + virLogBuffer[virLogEnd] = 0; /* * Update the log length, and if full move the start index */ -- 1.7.4
On Fri, Mar 18, 2011 at 08:31:22PM -0600, Eric Blake wrote:
Valgrind caught that our log wrap-around was going 1 past the end. Regression introduced in commit b16f47a; previously the buffer was static and size+1 bytes, but now it is dynamic and exactly size bytes.
* src/util/logging.c (virLogStr): Don't write past end of log. ---
An alternative would be to malloc one larger; but since the log is likely to be a page size multiple and large enough to be worth malloc using mmap, going one larger is likely to waste the bulk of a page. Also, I like always NUL-terminating the current end of the log (without including that NUL in the log length).
src/util/logging.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/logging.c b/src/util/logging.c index b972f8a..f4910ad 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -326,7 +326,7 @@ static void virLogStr(const char *str, int len) { return; if (len <= 0) len = strlen(str); - if (len > virLogSize) + if (len >= virLogSize) return; virLogLock();
@@ -336,13 +336,13 @@ static void virLogStr(const char *str, int len) { if (virLogEnd + len >= virLogSize) { tmp = virLogSize - virLogEnd; memcpy(&virLogBuffer[virLogEnd], str, tmp); - virLogBuffer[virLogSize] = 0; memcpy(&virLogBuffer[0], &str[tmp], len - tmp); virLogEnd = len - tmp; } else { memcpy(&virLogBuffer[virLogEnd], str, len); virLogEnd += len; } + virLogBuffer[virLogEnd] = 0; /* * Update the log length, and if full move the start index */
Well, I tend to prefer having a 0 at the end of character arrays in C, this can be useful for example when debugging, so slightly preferring one extra byte malloc for safety, either way not a big deal, but let's fix this, ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/21/2011 02:20 AM, Daniel Veillard wrote:
On Fri, Mar 18, 2011 at 08:31:22PM -0600, Eric Blake wrote:
Valgrind caught that our log wrap-around was going 1 past the end. Regression introduced in commit b16f47a; previously the buffer was static and size+1 bytes, but now it is dynamic and exactly size bytes.
* src/util/logging.c (virLogStr): Don't write past end of log. ---
An alternative would be to malloc one larger; but since the log is likely to be a page size multiple and large enough to be worth malloc using mmap, going one larger is likely to waste the bulk of a page. Also, I like always NUL-terminating the current end of the log (without including that NUL in the log length).
src/util/logging.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/logging.c b/src/util/logging.c index b972f8a..f4910ad 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -326,7 +326,7 @@ static void virLogStr(const char *str, int len) { return; if (len <= 0) len = strlen(str); - if (len > virLogSize) + if (len >= virLogSize) return; virLogLock();
@@ -336,13 +336,13 @@ static void virLogStr(const char *str, int len) { if (virLogEnd + len >= virLogSize) { tmp = virLogSize - virLogEnd; memcpy(&virLogBuffer[virLogEnd], str, tmp); - virLogBuffer[virLogSize] = 0; memcpy(&virLogBuffer[0], &str[tmp], len - tmp); virLogEnd = len - tmp; } else { memcpy(&virLogBuffer[virLogEnd], str, len); virLogEnd += len; } + virLogBuffer[virLogEnd] = 0; /* * Update the log length, and if full move the start index */
Well, I tend to prefer having a 0 at the end of character arrays in C, this can be useful for example when debugging, so slightly preferring one extra byte malloc for safety, either way not a big deal, but let's fix this,
I see two ways to do things - either malloc an extra byte (which will always be NUL from the malloc, so deleting 'virLogBuffer[virLogSize] = 0' is still okay), or by touching the rest of the code to always leave virLogBuffer[virLogSize-1] as NUL (more invasive, and caps the useful log to virLogSize-1). Do you want me to prepare a followup patch, and if so, for which of those two options?
ACK
I went ahead and pushed this as-is to avoid the invalid write; we can do further cleanup based on how we answer the above question if we want to guarantee a NUL byte at the end of the array for debugging purposes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Mar 21, 2011 at 10:24:05AM -0600, Eric Blake wrote:
On 03/21/2011 02:20 AM, Daniel Veillard wrote:
On Fri, Mar 18, 2011 at 08:31:22PM -0600, Eric Blake wrote: Well, I tend to prefer having a 0 at the end of character arrays in C, this can be useful for example when debugging, so slightly preferring one extra byte malloc for safety, either way not a big deal, but let's fix this,
I see two ways to do things - either malloc an extra byte (which will always be NUL from the malloc, so deleting 'virLogBuffer[virLogSize] = 0' is still okay), or by touching the rest of the code to always leave virLogBuffer[virLogSize-1] as NUL (more invasive, and caps the useful log to virLogSize-1). Do you want me to prepare a followup patch, and if so, for which of those two options?
it's really a minor issue, malloc'ing an extra byte sounds fine to me. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* src/util/logging.c (virLogStartup, virLogSetBufferSize): Over-allocate, so that a debugger can just print the circular buffer. Suggested by Daniel Veillard. ---
I see two ways to do things - either malloc an extra byte (which will always be NUL from the malloc, so deleting 'virLogBuffer[virLogSize] = 0' is still okay), or by touching the rest of the code to always leave virLogBuffer[virLogSize-1] as NUL (more invasive, and caps the useful log to virLogSize-1). Do you want me to prepare a followup patch, and if so, for which of those two options?
it's really a minor issue, malloc'ing an extra byte sounds fine to me.
src/util/logging.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/logging.c b/src/util/logging.c index f4910ad..48c0cfd 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -197,14 +197,14 @@ int virLogStartup(void) { virLogInitialized = 1; virLogLock(); - if (VIR_ALLOC_N(virLogBuffer, virLogSize) < 0) { + if (VIR_ALLOC_N(virLogBuffer, virLogSize + 1) < 0) { /* * The debug buffer is not a critical component, allow startup * even in case of failure to allocate it in case of a * configuration mistake. */ virLogSize = 64 * 1024; - if (VIR_ALLOC_N(virLogBuffer, virLogSize) < 0) { + if (VIR_ALLOC_N(virLogBuffer, virLogSize + 1) < 0) { pbm = "Failed to allocate debug buffer: deactivating debug log\n"; virLogSize = 0; } else { @@ -249,14 +249,14 @@ virLogSetBufferSize(int size) { oldsize = virLogSize; oldLogBuffer = virLogBuffer; - if (INT_MAX / 1024 < size) { + if (INT_MAX / 1024 <= size) { pbm = "Requested log size of %d kB too large\n"; ret = -1; goto error; } virLogSize = size * 1024; - if (VIR_ALLOC_N(virLogBuffer, virLogSize) < 0) { + if (VIR_ALLOC_N(virLogBuffer, virLogSize + 1) < 0) { pbm = "Failed to allocate debug buffer of %d kB\n"; virLogBuffer = oldLogBuffer; virLogSize = oldsize; -- 1.7.4
participants (2)
-
Daniel Veillard -
Eric Blake