| Message ID | 1361832890-40921-15-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
Hello, Eric 2013/2/26 Eric Sandeen <sandeen@redhat.com>: > If we exit with error we must free the allocated memory > to avoid a leak. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > btrfs-list.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/btrfs-list.c b/btrfs-list.c > index 851c059..8c3f84d 100644 > --- a/btrfs-list.c > +++ b/btrfs-list.c > @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, > * ref_tree = 0 indicates the subvolumes > * has been deleted. > */ > - if (!found->ref_tree) > + if (!found->ref_tree) { > + free(full_path); > return -ENOENT; > + } > int add_len = strlen(found->path); > > /* room for / and for null */ > @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, > * subvolume was deleted. > */ > found = root_tree_search(rl, next); > - if (!found) > + if (!found) { > + free(full_path); > return -ENOENT; > + } > } > > ri->full_path = full_path; > -- > 1.7.1 I think the patch is wrong; Here we return ENOENT, it means a subvolume/snapshot deletion happens. We just filter them in the filter_root, But the free work is done by the function all_subvolume_free.. so your modification will cause a double free.. Thanks, Wang > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/25/13 6:36 PM, Shilong Wang wrote: > Hello, Eric > > 2013/2/26 Eric Sandeen <sandeen@redhat.com>: >> If we exit with error we must free the allocated memory >> to avoid a leak. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> btrfs-list.c | 8 ++++++-- >> 1 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/btrfs-list.c b/btrfs-list.c >> index 851c059..8c3f84d 100644 >> --- a/btrfs-list.c >> +++ b/btrfs-list.c >> @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, >> * ref_tree = 0 indicates the subvolumes >> * has been deleted. >> */ >> - if (!found->ref_tree) >> + if (!found->ref_tree) { >> + free(full_path); >> return -ENOENT; >> + } >> int add_len = strlen(found->path); >> >> /* room for / and for null */ >> @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, >> * subvolume was deleted. >> */ >> found = root_tree_search(rl, next); >> - if (!found) >> + if (!found) { >> + free(full_path); >> return -ENOENT; >> + } >> } >> >> ri->full_path = full_path; >> -- >> 1.7.1 > > I think the patch is wrong; > Here we return ENOENT, it means a subvolume/snapshot deletion happens. > We just filter them in the filter_root, But the free work is done by > the function all_subvolume_free.. > so your modification will cause a double free.. Thanks for the review. I'll admit that when looking at too many of these static checker reports, sometimes things look obvious when they are actually subtle, and I've certainly made mistakes before. :) However, full_path location doesn't seem to be available outside the scope of this function unless we exit normally and do: ri->full_path = full_path; return 0; } If we exit early at the -ENOENT points, it seems that full_path is leaked; there are no other references to it. Am I missing something? Thanks, -Eric > Thanks, > Wang > >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 25, 2013 at 10:36:41PM -0600, Eric Sandeen wrote: > On 2/25/13 6:36 PM, Shilong Wang wrote: > >> --- a/btrfs-list.c > >> +++ b/btrfs-list.c > >> @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, > >> * ref_tree = 0 indicates the subvolumes > >> * has been deleted. > >> */ > >> - if (!found->ref_tree) > >> + if (!found->ref_tree) { > >> + free(full_path); > >> return -ENOENT; > >> + } > >> int add_len = strlen(found->path); > >> > >> /* room for / and for null */ > >> @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, > >> * subvolume was deleted. > >> */ > >> found = root_tree_search(rl, next); > >> - if (!found) > >> + if (!found) { > >> + free(full_path); > >> return -ENOENT; > >> + } > >> } > >> > >> ri->full_path = full_path; > >> -- > >> 1.7.1 > > > > I think the patch is wrong; > > Here we return ENOENT, it means a subvolume/snapshot deletion happens. > > We just filter them in the filter_root, But the free work is done by > > the function all_subvolume_free.. > > so your modification will cause a double free.. > > Thanks for the review. I'll admit that when looking at too many of > these static checker reports, sometimes things look obvious when > they are actually subtle, and I've certainly made mistakes before. :) > > However, full_path location doesn't seem to be available outside the > scope of this function unless we exit normally and do: > > ri->full_path = full_path; > > return 0; > } > > If we exit early at the -ENOENT points, it seems that full_path > is leaked; there are no other references to it. I agree with you, the freed value is local. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Eric, David 2013/2/27 David Sterba <dsterba@suse.cz>: > On Mon, Feb 25, 2013 at 10:36:41PM -0600, Eric Sandeen wrote: >> On 2/25/13 6:36 PM, Shilong Wang wrote: >> >> --- a/btrfs-list.c >> >> +++ b/btrfs-list.c >> >> @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, >> >> * ref_tree = 0 indicates the subvolumes >> >> * has been deleted. >> >> */ >> >> - if (!found->ref_tree) >> >> + if (!found->ref_tree) { >> >> + free(full_path); >> >> return -ENOENT; >> >> + } >> >> int add_len = strlen(found->path); >> >> >> >> /* room for / and for null */ >> >> @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, >> >> * subvolume was deleted. >> >> */ >> >> found = root_tree_search(rl, next); >> >> - if (!found) >> >> + if (!found) { >> >> + free(full_path); >> >> return -ENOENT; >> >> + } >> >> } >> >> >> >> ri->full_path = full_path; >> >> -- >> >> 1.7.1 >> > >> > I think the patch is wrong; >> > Here we return ENOENT, it means a subvolume/snapshot deletion happens. >> > We just filter them in the filter_root, But the free work is done by >> > the function all_subvolume_free.. >> > so your modification will cause a double free.. >> >> Thanks for the review. I'll admit that when looking at too many of >> these static checker reports, sometimes things look obvious when >> they are actually subtle, and I've certainly made mistakes before. :) >> >> However, full_path location doesn't seem to be available outside the >> scope of this function unless we exit normally and do: >> >> ri->full_path = full_path; >> >> return 0; >> } >> >> If we exit early at the -ENOENT points, it seems that full_path >> is leaked; there are no other references to it. I looked the code carefully, i was wrong before.. Agree with the patch Thanks, Wang > > I agree with you, the freed value is local. > > david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/btrfs-list.c b/btrfs-list.c index 851c059..8c3f84d 100644 --- a/btrfs-list.c +++ b/btrfs-list.c @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, * ref_tree = 0 indicates the subvolumes * has been deleted. */ - if (!found->ref_tree) + if (!found->ref_tree) { + free(full_path); return -ENOENT; + } int add_len = strlen(found->path); /* room for / and for null */ @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri, * subvolume was deleted. */ found = root_tree_search(rl, next); - if (!found) + if (!found) { + free(full_path); return -ENOENT; + } } ri->full_path = full_path;
If we exit with error we must free the allocated memory to avoid a leak. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- btrfs-list.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)