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

> 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.