3:19 a.m.
As Pino's comment, we should take advantage of
macro CLEANUP_FREE_STRING_LIST
v4: remove some redundant strdup
v3: fix test case failure
v2: properly initialize lines
Chen Hanxiao (3):
do_btrfs_qgroup_show: fix a bad return value
do_btrfs_subvolume_list: fix a bad return value
btrfs: use CLEANUP_FREE_STRING_LIST for list free
daemon/btrfs.c | 70 +++++++++++++++++++++++++++++++---------------------------
1 file changed, 37 insertions(+), 33 deletions(-)
--
2.1.0
3:19 a.m.
New subject: [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value
We should not use tmp lines buffer as return value,
for lines buffer will be freed.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
v4: take advantage of sscanf's '%m'.
v3: fix test case failure
daemon/btrfs.c | 40 ++++++++++++++++++----------------------
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 7b14bac..8d03caa 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1249,7 +1249,7 @@ do_btrfs_qgroup_show (const char *path)
CLEANUP_FREE char *err = NULL;
CLEANUP_FREE char *out = NULL;
int r;
- char **lines;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
path_buf = sysroot_path (path);
if (path_buf == NULL) {
@@ -1275,17 +1275,19 @@ do_btrfs_qgroup_show (const char *path)
if (!lines)
return NULL;
- /* line 0 and 1 are:
+ /* Output of `btrfs qgroup show' is like:
+ *
+ * qgroupid rfer excl
+ * -------- ---- ----
+ * 0/5 9249849344 9249849344
*
- * qgroupid rfer excl
- * -------- ---- ----
*/
size_t nr_qgroups = count_strings (lines) - 2;
guestfs_int_btrfsqgroup_list *ret = NULL;
ret = malloc (sizeof *ret);
if (!ret) {
reply_with_perror ("malloc");
- goto error;
+ return NULL;
}
ret->guestfs_int_btrfsqgroup_list_len = nr_qgroups;
@@ -1293,38 +1295,32 @@ do_btrfs_qgroup_show (const char *path)
calloc (nr_qgroups, sizeof (struct guestfs_int_btrfsqgroup));
if (ret->guestfs_int_btrfsqgroup_list_val == NULL) {
reply_with_perror ("malloc");
- goto error;
+ free (ret);
+ return NULL;
}
for (i = 0; i < nr_qgroups; ++i) {
char *line = lines[i + 2];
struct guestfs_int_btrfsqgroup *this =
&ret->guestfs_int_btrfsqgroup_list_val[i];
- uint64_t dummy1, dummy2;
- char *p;
- if (sscanf (line, "%" SCNu64 "/%" SCNu64 " %" SCNu64
" %" SCNu64,
- &dummy1, &dummy2, &this->btrfsqgroup_rfer,
- &this->btrfsqgroup_excl) != 4) {
+ if (sscanf (line, "%m[0-9/] %" SCNu64 " %" SCNu64,
+ &this->btrfsqgroup_id, &this->btrfsqgroup_rfer,
+ &this->btrfsqgroup_excl) != 3) {
reply_with_error ("cannot parse output of qgroup show command: %s",
line);
goto error;
}
- p = strchr(line, ' ');
- if (!p) {
- reply_with_error ("truncated line: %s", line);
- goto error;
- }
- *p = '\0';
- this->btrfsqgroup_id = line;
}
- free (lines);
return ret;
error:
- free_stringslen (lines, nr_qgroups + 2);
- if (ret)
- free (ret->guestfs_int_btrfsqgroup_list_val);
+ for (i = 0; i < nr_qgroups; ++i) {
+ struct guestfs_int_btrfsqgroup *this =
+ &ret->guestfs_int_btrfsqgroup_list_val[i];
+ free (this->btrfsqgroup_id);
+ }
+ free (ret->guestfs_int_btrfsqgroup_list_val);
free (ret);
return NULL;
--
2.1.0
9:27 a.m.
New subject: [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value
On Wednesday 17 June 2015 16:19:31 Chen Hanxiao wrote:
We should not use tmp lines buffer as return value,
for lines buffer will be freed.
s/tmp/temporary/
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
v4: take advantage of sscanf's '%m'.
v3: fix test case failure
daemon/btrfs.c | 40 ++++++++++++++++++----------------------
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 7b14bac..8d03caa 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1249,7 +1249,7 @@ do_btrfs_qgroup_show (const char *path)
CLEANUP_FREE char *err = NULL;
CLEANUP_FREE char *out = NULL;
int r;
- char **lines;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
path_buf = sysroot_path (path);
if (path_buf == NULL) {
@@ -1275,17 +1275,19 @@ do_btrfs_qgroup_show (const char *path)
if (!lines)
return NULL;
- /* line 0 and 1 are:
+ /* Output of `btrfs qgroup show' is like:
+ *
+ * qgroupid rfer excl
+ * -------- ---- ----
+ * 0/5 9249849344 9249849344
*
- * qgroupid rfer excl
- * -------- ---- ----
*/
size_t nr_qgroups = count_strings (lines) - 2;
guestfs_int_btrfsqgroup_list *ret = NULL;
ret = malloc (sizeof *ret);
if (!ret) {
reply_with_perror ("malloc");
- goto error;
+ return NULL;
}
ret->guestfs_int_btrfsqgroup_list_len = nr_qgroups;
@@ -1293,38 +1295,32 @@ do_btrfs_qgroup_show (const char *path)
calloc (nr_qgroups, sizeof (struct guestfs_int_btrfsqgroup));
if (ret->guestfs_int_btrfsqgroup_list_val == NULL) {
reply_with_perror ("malloc");
- goto error;
+ free (ret);
+ return NULL;
}
You don't need the change here, if later in the "error:" label you keep
the "if (ret)" condition.
for (i = 0; i < nr_qgroups; ++i) {
char *line = lines[i + 2];
struct guestfs_int_btrfsqgroup *this =
&ret->guestfs_int_btrfsqgroup_list_val[i];
- uint64_t dummy1, dummy2;
- char *p;
- if (sscanf (line, "%" SCNu64 "/%" SCNu64 " %" SCNu64
" %" SCNu64,
- &dummy1, &dummy2, &this->btrfsqgroup_rfer,
- &this->btrfsqgroup_excl) != 4) {
+ if (sscanf (line, "%m[0-9/] %" SCNu64 " %" SCNu64,
+ &this->btrfsqgroup_id, &this->btrfsqgroup_rfer,
+ &this->btrfsqgroup_excl) != 3) {
reply_with_error ("cannot parse output of qgroup show command: %s",
line);
goto error;
}
- p = strchr(line, ' ');
- if (!p) {
- reply_with_error ("truncated line: %s", line);
- goto error;
- }
- *p = '\0';
- this->btrfsqgroup_id = line;
}
- free (lines);
return ret;
error:
- free_stringslen (lines, nr_qgroups + 2);
- if (ret)
- free (ret->guestfs_int_btrfsqgroup_list_val);
+ for (i = 0; i < nr_qgroups; ++i) {
+ struct guestfs_int_btrfsqgroup *this =
+ &ret->guestfs_int_btrfsqgroup_list_val[i];
+ free (this->btrfsqgroup_id);
No need to save "this", just
free (ret->guestfs_int_btrfsqgroup_list_val[i].btrfsqgroup_id);
should be enough (still well under 80 characters :) ).
Thanks,
--
Pino Toscano
8:52 p.m.
New subject: [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value
-----Original Message-----
From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On
Behalf Of Pino Toscano
Sent: Wednesday, June 17, 2015 10:28 PM
To: libguestfs(a)redhat.com
Subject: Re: [Libguestfs] [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return
value
On Wednesday 17 June 2015 16:19:31 Chen Hanxiao wrote:
> We should not use tmp lines buffer as return value,
> for lines buffer will be freed.
s/tmp/temporary/
Fine.
> Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
> ---
> v4: take advantage of sscanf's '%m'.
> v3: fix test case failure
>
> daemon/btrfs.c | 40 ++++++++++++++++++----------------------
> 1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 7b14bac..8d03caa 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -1249,7 +1249,7 @@ do_btrfs_qgroup_show (const char *path)
> CLEANUP_FREE char *err = NULL;
> CLEANUP_FREE char *out = NULL;
> int r;
> - char **lines;
> + CLEANUP_FREE_STRING_LIST char **lines = NULL;
>
> path_buf = sysroot_path (path);
> if (path_buf == NULL) {
> @@ -1275,17 +1275,19 @@ do_btrfs_qgroup_show (const char *path)
> if (!lines)
> return NULL;
>
> - /* line 0 and 1 are:
> + /* Output of `btrfs qgroup show' is like:
> + *
> + * qgroupid rfer excl
> + * -------- ---- ----
> + * 0/5 9249849344 9249849344
> *
> - * qgroupid rfer excl
> - * -------- ---- ----
> */
> size_t nr_qgroups = count_strings (lines) - 2;
> guestfs_int_btrfsqgroup_list *ret = NULL;
> ret = malloc (sizeof *ret);
> if (!ret) {
> reply_with_perror ("malloc");
> - goto error;
> + return NULL;
> }
>
> ret->guestfs_int_btrfsqgroup_list_len = nr_qgroups;
> @@ -1293,38 +1295,32 @@ do_btrfs_qgroup_show (const char *path)
> calloc (nr_qgroups, sizeof (struct guestfs_int_btrfsqgroup));
> if (ret->guestfs_int_btrfsqgroup_list_val == NULL) {
> reply_with_perror ("malloc");
> - goto error;
> + free (ret);
> + return NULL;
> }
You don't need the change here, if later in the "error:" label you keep
the "if (ret)" condition.
OK.
Although it already works fine.
Will changed in next version.
Regards,
- Chen
> for (i = 0; i < nr_qgroups; ++i) {
> char *line = lines[i + 2];
> struct guestfs_int_btrfsqgroup *this =
> &ret->guestfs_int_btrfsqgroup_list_val[i];
> - uint64_t dummy1, dummy2;
> - char *p;
>
> - if (sscanf (line, "%" SCNu64 "/%" SCNu64 " %"
SCNu64 " %" SCNu64,
> - &dummy1, &dummy2, &this->btrfsqgroup_rfer,
> - &this->btrfsqgroup_excl) != 4) {
> + if (sscanf (line, "%m[0-9/] %" SCNu64 " %" SCNu64,
> + &this->btrfsqgroup_id, &this->btrfsqgroup_rfer,
> + &this->btrfsqgroup_excl) != 3) {
> reply_with_error ("cannot parse output of qgroup show command: %s",
line);
> goto error;
> }
> - p = strchr(line, ' ');
> - if (!p) {
> - reply_with_error ("truncated line: %s", line);
> - goto error;
> - }
> - *p = '\0';
> - this->btrfsqgroup_id = line;
> }
>
> - free (lines);
> return ret;
>
> error:
> - free_stringslen (lines, nr_qgroups + 2);
> - if (ret)
> - free (ret->guestfs_int_btrfsqgroup_list_val);
> + for (i = 0; i < nr_qgroups; ++i) {
> + struct guestfs_int_btrfsqgroup *this =
> + &ret->guestfs_int_btrfsqgroup_list_val[i];
> + free (this->btrfsqgroup_id);
No need to save "this", just
free (ret->guestfs_int_btrfsqgroup_list_val[i].btrfsqgroup_id);
should be enough (still well under 80 characters :) ).
Thanks,
--
Pino Toscano
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
3:19 a.m.
New subject: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
don't return a value which is to be freed.
v4: use strndup
v3: v3: fix test case failure
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
daemon/btrfs.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 8d03caa..5361984 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -409,7 +409,7 @@ umount (char *fs_buf, const mountable_t *fs)
guestfs_int_btrfssubvolume_list *
do_btrfs_subvolume_list (const mountable_t *fs)
{
- char **lines;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
size_t i = 0;
const size_t MAX_ARGS = 64;
const char *argv[MAX_ARGS];
@@ -472,7 +472,7 @@ do_btrfs_subvolume_list (const mountable_t *fs)
ret = malloc (sizeof *ret);
if (!ret) {
reply_with_perror ("malloc");
- goto error;
+ return NULL;
}
ret->guestfs_int_btrfssubvolume_list_len = nr_subvolumes;
@@ -480,7 +480,8 @@ do_btrfs_subvolume_list (const mountable_t *fs)
calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume));
if (ret->guestfs_int_btrfssubvolume_list_val == NULL) {
reply_with_perror ("malloc");
- goto error;
+ free (ret);
+ return NULL;
}
const char *errptr;
@@ -530,20 +531,27 @@ do_btrfs_subvolume_list (const mountable_t *fs)
#undef XSTRTOU64
- memmove (line, line + ovector[6], ovector[7] - ovector[6] + 1);
- this->btrfssubvolume_path = line;
+ this->btrfssubvolume_path =
+ strndup (line + ovector[6], ovector[7] - ovector[6]);
+ if (this->btrfssubvolume_path == NULL)
+ goto error;
}
- free (lines);
pcre_free (re);
return ret;
error:
- free_stringslen (lines, nr_subvolumes);
- if (ret) free (ret->guestfs_int_btrfssubvolume_list_val);
+ for (i = 0; i < nr_subvolumes; ++i) {
+ struct guestfs_int_btrfssubvolume *this =
+ &ret->guestfs_int_btrfssubvolume_list_val[i];
+ free (this->btrfssubvolume_path);
+ }
+ free (ret->guestfs_int_btrfssubvolume_list_val);
free (ret);
- if (re) pcre_free (re);
+
+ if (re)
+ pcre_free (re);
return NULL;
}
--
2.1.0
9:44 a.m.
New subject: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
On Wednesday 17 June 2015 16:19:32 Chen Hanxiao wrote:
don't return a value which is to be freed.
v4: use strndup
v3: v3: fix test case failure
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
daemon/btrfs.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 8d03caa..5361984 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -409,7 +409,7 @@ umount (char *fs_buf, const mountable_t *fs)
guestfs_int_btrfssubvolume_list *
do_btrfs_subvolume_list (const mountable_t *fs)
{
- char **lines;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
size_t i = 0;
const size_t MAX_ARGS = 64;
const char *argv[MAX_ARGS];
@@ -472,7 +472,7 @@ do_btrfs_subvolume_list (const mountable_t *fs)
ret = malloc (sizeof *ret);
if (!ret) {
reply_with_perror ("malloc");
- goto error;
+ return NULL;
}
ret->guestfs_int_btrfssubvolume_list_len = nr_subvolumes;
@@ -480,7 +480,8 @@ do_btrfs_subvolume_list (const mountable_t *fs)
calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume));
if (ret->guestfs_int_btrfssubvolume_list_val == NULL) {
reply_with_perror ("malloc");
- goto error;
+ free (ret);
+ return NULL;
}
You don't need the change here, if later in the "error:" label you keep
the "if (ret)" condition.
const char *errptr;
@@ -530,20 +531,27 @@ do_btrfs_subvolume_list (const mountable_t *fs)
#undef XSTRTOU64
- memmove (line, line + ovector[6], ovector[7] - ovector[6] + 1);
- this->btrfssubvolume_path = line;
+ this->btrfssubvolume_path =
+ strndup (line + ovector[6], ovector[7] - ovector[6]);
+ if (this->btrfssubvolume_path == NULL)
+ goto error;
}
- free (lines);
pcre_free (re);
return ret;
error:
- free_stringslen (lines, nr_subvolumes);
- if (ret) free (ret->guestfs_int_btrfssubvolume_list_val);
+ for (i = 0; i < nr_subvolumes; ++i) {
+ struct guestfs_int_btrfssubvolume *this =
+ &ret->guestfs_int_btrfssubvolume_list_val[i];
+ free (this->btrfssubvolume_path);
No need to save "this", just
free (ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path);
should be enough.
Thanks,
--
Pino Toscano
12:14 a.m.
New subject: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
Hi, Pino
-----Original Message-----
From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On
Behalf Of Pino Toscano
Sent: Wednesday, June 17, 2015 10:44 PM
To: libguestfs(a)redhat.com
Subject: Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return
value
On Wednesday 17 June 2015 16:19:32 Chen Hanxiao wrote:
> don't return a value which is to be freed.
>
> v4: use strndup
> v3: v3: fix test case failure
> Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
> ---
> daemon/btrfs.c | 26 +++++++++++++++++---------
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 8d03caa..5361984 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -409,7 +409,7 @@ umount (char *fs_buf, const mountable_t *fs)
> guestfs_int_btrfssubvolume_list *
> do_btrfs_subvolume_list (const mountable_t *fs)
> {
> - char **lines;
> + CLEANUP_FREE_STRING_LIST char **lines = NULL;
> size_t i = 0;
> const size_t MAX_ARGS = 64;
> const char *argv[MAX_ARGS];
> @@ -472,7 +472,7 @@ do_btrfs_subvolume_list (const mountable_t *fs)
> ret = malloc (sizeof *ret);
> if (!ret) {
> reply_with_perror ("malloc");
> - goto error;
> + return NULL;
> }
>
> ret->guestfs_int_btrfssubvolume_list_len = nr_subvolumes;
> @@ -480,7 +480,8 @@ do_btrfs_subvolume_list (const mountable_t *fs)
> calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume));
> if (ret->guestfs_int_btrfssubvolume_list_val == NULL) {
> reply_with_perror ("malloc");
> - goto error;
> + free (ret);
> + return NULL;
> }
You don't need the change here, if later in the "error:" label you keep
the "if (ret)" condition.
If we succeeded at malloc(3) but failed at calloc(3),
we will goto error.
At this time we've got a space with uninitialized data because of malloc(3),
but no space for guestfs_int_btrfsqgroup_list_val.
When processing in label error, we could not know:
ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path
is a valid address.
1) One solution is use calloc to replace the first malloc.
Then:
if (ret-> guestfs_int_btrfssubvolume_list_val)
for (...)
It costs more codes.
2) use the current solution
I think the process in this patch should be a choice.
How do you think?
Regards,
- Chen
> const char *errptr;
> @@ -530,20 +531,27 @@ do_btrfs_subvolume_list (const mountable_t *fs)
>
> #undef XSTRTOU64
>
> - memmove (line, line + ovector[6], ovector[7] - ovector[6] + 1);
> - this->btrfssubvolume_path = line;
> + this->btrfssubvolume_path =
> + strndup (line + ovector[6], ovector[7] - ovector[6]);
> + if (this->btrfssubvolume_path == NULL)
> + goto error;
> }
>
> - free (lines);
> pcre_free (re);
>
> return ret;
>
> error:
> - free_stringslen (lines, nr_subvolumes);
> - if (ret) free (ret->guestfs_int_btrfssubvolume_list_val);
> + for (i = 0; i < nr_subvolumes; ++i) {
> + struct guestfs_int_btrfssubvolume *this =
> + &ret->guestfs_int_btrfssubvolume_list_val[i];
> + free (this->btrfssubvolume_path);
No need to save "this", just
free (ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path);
should be enough.
Thanks,
--
Pino Toscano
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
3:32 a.m.
New subject: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
Hi,
On Thursday 18 June 2015 05:14:46 Chen, Hanxiao wrote:
> -----Original Message-----
> From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On
> Behalf Of Pino Toscano
> Sent: Wednesday, June 17, 2015 10:44 PM
> To: libguestfs(a)redhat.com
> Subject: Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return
> value
>
> On Wednesday 17 June 2015 16:19:32 Chen Hanxiao wrote:
> > don't return a value which is to be freed.
> >
> > v4: use strndup
> > v3: v3: fix test case failure
> > Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
> > ---
> > daemon/btrfs.c | 26 +++++++++++++++++---------
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> > index 8d03caa..5361984 100644
> > --- a/daemon/btrfs.c
> > +++ b/daemon/btrfs.c
> > @@ -409,7 +409,7 @@ umount (char *fs_buf, const mountable_t *fs)
> > guestfs_int_btrfssubvolume_list *
> > do_btrfs_subvolume_list (const mountable_t *fs)
> > {
> > - char **lines;
> > + CLEANUP_FREE_STRING_LIST char **lines = NULL;
> > size_t i = 0;
> > const size_t MAX_ARGS = 64;
> > const char *argv[MAX_ARGS];
> > @@ -472,7 +472,7 @@ do_btrfs_subvolume_list (const mountable_t *fs)
> > ret = malloc (sizeof *ret);
> > if (!ret) {
> > reply_with_perror ("malloc");
> > - goto error;
> > + return NULL;
> > }
> >
> > ret->guestfs_int_btrfssubvolume_list_len = nr_subvolumes;
> > @@ -480,7 +480,8 @@ do_btrfs_subvolume_list (const mountable_t *fs)
> > calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume));
> > if (ret->guestfs_int_btrfssubvolume_list_val == NULL) {
> > reply_with_perror ("malloc");
> > - goto error;
> > + free (ret);
> > + return NULL;
> > }
>
> You don't need the change here, if later in the "error:" label you
keep
> the "if (ret)" condition.
>
If we succeeded at malloc(3) but failed at calloc(3),
we will goto error.
At this time we've got a space with uninitialized data because of malloc(3),
but no space for guestfs_int_btrfsqgroup_list_val.
When processing in label error, we could not know:
ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path
is a valid address.
1) One solution is use calloc to replace the first malloc.
Then:
if (ret-> guestfs_int_btrfssubvolume_list_val)
for (...)
It costs more codes.
2) use the current solution
I think the process in this patch should be a choice.
How do you think?
If calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume))
fails, then ret->guestfs_int_btrfssubvolume_list_val is already a null
pointer, which means you can just check for it as you do in (1) above,
with no need to switch from malloc to calloc.
The other alternative is to use more labels for error conditions in a
symmetric way, like:
ptr1 = malloc (...);
if (ptr1 == NULL)
goto error1;
ptr1->subptr1 = malloc (...);
if (ptr1->subptr1 == NULL)
goto error2;
ptr1->subptr2 = malloc (...);
if (ptr1->subptr2 == NULL)
goto error3;
...
error3:
free (ptr1->subptr1);
error2:
free (ptr1);
error1:
...
--
Pino Toscano
4:02 a.m.
New subject: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
Hi,
-----Original Message-----
From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On
Behalf Of Pino Toscano
Sent: Thursday, June 18, 2015 4:32 PM
To: libguestfs(a)redhat.com
Subject: Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return
value
[snip]
> >
>
> If we succeeded at malloc(3) but failed at calloc(3),
> we will goto error.
>
> At this time we've got a space with uninitialized data because of malloc(3),
> but no space for guestfs_int_btrfsqgroup_list_val.
> When processing in label error, we could not know:
> ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path
> is a valid address.
>
> 1) One solution is use calloc to replace the first malloc.
> Then:
> if (ret-> guestfs_int_btrfssubvolume_list_val)
> for (...)
>
> It costs more codes.
>
> 2) use the current solution
>
> I think the process in this patch should be a choice.
> How do you think?
If calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume))
fails, then ret->guestfs_int_btrfssubvolume_list_val is already a null
pointer, which means you can just check for it as you do in (1) above,
with no need to switch from malloc to calloc.
The other alternative is to use more labels for error conditions in a
symmetric way, like:
ptr1 = malloc (...);
if (ptr1 == NULL)
goto error1;
ptr1->subptr1 = malloc (...);
if (ptr1->subptr1 == NULL)
goto error2;
ptr1->subptr2 = malloc (...);
if (ptr1->subptr2 == NULL)
goto error3;
...
error3:
free (ptr1->subptr1);
error2:
free (ptr1);
error1:
...
Thanks for your detail clarification and kindly patience.
Although my patch could work, I'll send a new version as your comments.
Regards,
- Chen
3:19 a.m.
New subject: [PATCH v4 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
daemon/btrfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 5361984..88be9ec 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1692,10 +1692,10 @@ do_btrfs_balance_status (const char *path)
size_t i = 0;
CLEANUP_FREE char *path_buf = NULL;
CLEANUP_FREE char *err = NULL;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
char *out;
int r;
guestfs_int_btrfsbalance *ret;
- char **lines;
size_t nlines;
const char *errptr;
int erroffset;
@@ -1817,10 +1817,10 @@ do_btrfs_scrub_status (const char *path)
size_t i = 0;
CLEANUP_FREE char *path_buf = NULL;
CLEANUP_FREE char *err = NULL;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
char *out;
int r;
guestfs_int_btrfsscrub *ret;
- char **lines;
path_buf = sysroot_path (path);
if (path_buf == NULL) {
--
2.1.0
4:08 a.m.
New subject: [PATCH v4 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
On Wednesday 17 June 2015 16:19:33 Chen Hanxiao wrote:
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
daemon/btrfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 5361984..88be9ec 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1692,10 +1692,10 @@ do_btrfs_balance_status (const char *path)
size_t i = 0;
CLEANUP_FREE char *path_buf = NULL;
CLEANUP_FREE char *err = NULL;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
char *out;
int r;
guestfs_int_btrfsbalance *ret;
- char **lines;
size_t nlines;
const char *errptr;
int erroffset;
@@ -1817,10 +1817,10 @@ do_btrfs_scrub_status (const char *path)
size_t i = 0;
CLEANUP_FREE char *path_buf = NULL;
CLEANUP_FREE char *err = NULL;
+ CLEANUP_FREE_STRING_LIST char **lines = NULL;
char *out;
int r;
guestfs_int_btrfsscrub *ret;
- char **lines;
path_buf = sysroot_path (path);
if (path_buf == NULL) {
LGTM, pushed.
Thanks,
--
Pino Toscano
3445
days inactive
3446
days old
10 comments
3 participants
participants (3)
-
Chen Hanxiao -
Chen, Hanxiao
-
Pino Toscano