[libvirt] [PATCH] logging: fix off-by-one bug

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110321/abbaebc0/attachment-0001.sig>