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;

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.