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 at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/