Hello, Eric, David 2013/2/27 David Sterba <dste...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html