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

[Eric Blake]

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:

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

>  
>          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] == '.') {
>              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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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