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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list