Re: [PATCH 14/17] btrfs-progs: fix mem leak in resolve_root

[Eric Sandeen <sandeen@redhat.com>]

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