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