[libvirt] [PATCH] cgroup: Free line even if no characters were read

[Ján Tomko]

On 07/17/2013 04:03 AM, Eric Blake wrote:
> On 07/16/2013 06:14 AM, Ján Tomko wrote:
>> Even if getline doesn't read any characters it allocates a buffer.
>>
>> ==404== 120 bytes in 1 blocks are definitely lost in loss record 1,344 of 1,671
>> ==404==    at 0x4C2C71B: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==404==    by 0x906F862: getdelim (iogetdelim.c:68)
>> ==404==    by 0x52A48FB: virCgroupPartitionNeedsEscaping (vircgroup.c:1136)
>> ==404==    by 0x52A0FB4: virCgroupPartitionEscape (vircgroup.c:1171)
>> ==404==    by 0x52A0EA4: virCgroupNewDomainPartition (vircgroup.c:1450)
>>
>> Introduced by f366273.
>> ---
>>
>> Can STRPREFIX(path, line) be possibly true if tmp is NULL?
>> path[NULL - line] would be accessed in that case.
> 
> [1] good question; answer below (you didn't ask quite the right
> question, but it made me read the code - both the original code and this
> patch are broken; we need a v2).
> 
>>
>>  src/util/vircgroup.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
>> index 5a98393..2419d80 100644
>> --- a/src/util/vircgroup.c
>> +++ b/src/util/vircgroup.c
>> @@ -1136,38 +1136,38 @@ static int virCgroupPartitionNeedsEscaping(const char *path)
>>      while (getline(&line, &len, fp) > 0) {
>>          if (STRPREFIX(line, "#subsys_name")) {
>>              VIR_FREE(line);
> 
> This VIR_FREE(line) is spurious, and should be deleted.  The whole point
> of getline() is that it reuses a malloc'd buffer, possibly realloc()ing
> it to be larger, to minimize the malloc overhead.  But by freeing every
> iteration of the loop, we've lost that advantage.
> 
>>              continue;
>>          }
>>          char *tmp = strchr(line, ' ');
>>          if (tmp)
>>              *tmp = '\0';
>>          len = tmp - line;
> 
> This is bogus.  If tmp is NULL, then len is extremely large, and will
> mess up the next iteration call to getline().  However, I could live with:

It doesn't mess it up right now, since we set line to NULL every time.

> 
> if (tmp) {
>     *tmp = '\0';
>     len = tmp - line;
> }
> 
> which is slightly sub-optimal (it makes the next getline() call think it
> needs to call realloc(), when it really might already be large enough),
> but otherwise harmless (realloc() doesn't pay attention to *len, and is
> smart enough to be a no-op if the new size is no bigger than the
> existing real size).

This would leave len untouched if no space was found and tmp is NULL..

> 
>>  
>>          if (STRPREFIX(path, line) &&
> 
> [1] To answer your question, STRPREFIX() is unsafe to call on NULL (it
> is a macro that boils down to strncmp, which must NOT have null
> arguments).  But we are guaranteed that path and line are non-null here.
> 
>>              path[len] == '.') {

.. and path[len] would off by one instead of ~2^64.

>>              ret = 1;
>> -            VIR_FREE(line);
>>              goto cleanup;
>>          }
>>          VIR_FREE(line);
> 
> This is another VIR_FREE that could be safely nuked, given the semantics
> of readline().
> 
>>      }
>>  
>>      if (ferror(fp)) {
>>          ret = -EIO;
>>          goto cleanup;
>>      }
>>  
>>  cleanup:
>> +    VIR_FREE(line);
> 
> Yes, I agree that we need this, but we also need to fix the other bugs
> in the area.  Looking forward to v2.
> 

I've sent v2 as 'cgroup: reuse buffer for getline'

Jan