[libvirt] [PATCH] cgroup: Free line even if no characters were read
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. 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); continue; } char *tmp = strchr(line, ' '); if (tmp) *tmp = '\0'; len = tmp - line; if (STRPREFIX(path, line) && path[len] == '.') { ret = 1; - VIR_FREE(line); goto cleanup; } VIR_FREE(line); } if (ferror(fp)) { ret = -EIO; goto cleanup; } cleanup: + VIR_FREE(line); VIR_FORCE_FCLOSE(fp); return ret; } static int virCgroupPartitionEscape(char **path) { size_t len = strlen(*path) + 1; int rc; char escape = '_'; if ((rc = virCgroupPartitionNeedsEscaping(*path)) <= 0) return rc; -- 1.8.1.5
On 07/16/2013 08:14 PM, 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.
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); continue; } char *tmp = strchr(line, ' '); if (tmp) *tmp = '\0'; len = tmp - line;
if (STRPREFIX(path, line) && path[len] == '.') { ret = 1; - VIR_FREE(line); goto cleanup; } VIR_FREE(line); }
if (ferror(fp)) { ret = -EIO; goto cleanup; }
cleanup: + VIR_FREE(line); VIR_FORCE_FCLOSE(fp); return ret; }
static int virCgroupPartitionEscape(char **path) { size_t len = strlen(*path) + 1; int rc; char escape = '_';
if ((rc = virCgroupPartitionNeedsEscaping(*path)) <= 0) return rc;
ACK, I can reproduce the memory leak.
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
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
participants (3)
-
Eric Blake -
Guannan Ren -
Ján Tomko