Re: [libvirt] [PATCH] cgroup: Free line even if no characters were read
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