[Ocfs2-tools-devel] [PATCH] mkfs.ocfs2: fix memory leak problems in main()

[Eric Ren]

Hi,

On 08/26/2016 11:52 AM, piaojun wrote:
> Hi Eric Ren,
>
> On 2016-8-26 11:24, Eric Ren wrote:
>> Hi again,
>>
>> On 08/25/2016 04:40 PM, piaojun wrote:
>>> 'root_dir', 'system_dir', 'orphan_dir', 'record' and 's' need to be
>>> freed at the end of main().
>>>
>>> Signed-off-by: Jun Piao <piaojun at huawei.com>
>>> ---
>>>    mkfs.ocfs2/mkfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 47 insertions(+)
>>>
>>> diff --git a/mkfs.ocfs2/mkfs.c b/mkfs.ocfs2/mkfs.c
>>> index c361fda..321ba67 100644
>>> --- a/mkfs.ocfs2/mkfs.c
>>> +++ b/mkfs.ocfs2/mkfs.c
>>> @@ -21,6 +21,7 @@
>>>    #include "mkfs.h"
>>>      static State *get_state(int argc, char **argv);
>>> +static void free_state(State *s);
>>>    static int get_number(char *arg, uint64_t *res);
>>>    static void parse_journal_opts(char *progname, const char *opts,
>>>                       uint64_t *journal_size_in_bytes,
>>> @@ -46,6 +47,7 @@ static int alloc_from_bitmap(State *s, uint64_t num_bits, AllocBitmap *bitmap,
>>>                     uint64_t *start, uint64_t *num);
>>>    static uint64_t alloc_inode(State *s, uint16_t *suballoc_bit);
>>>    static DirData *alloc_directory(State *s);
>>> +static void free_directory(DirData *dir);
>>>    static void add_entry_to_directory(State *s, DirData *dir, char *name,
>>>                       uint64_t byte_off, uint8_t type);
>>>    static uint32_t blocks_needed(State *s);
>>> @@ -76,6 +78,7 @@ static AllocGroup * initialize_alloc_group(State *s, const char *name,
>>>                           uint64_t blkno,
>>>                           uint16_t chain, uint16_t cpg,
>>>                           uint16_t bpc);
>>> +static void free_alloc_group(AllocGroup *group);
>>>    static void index_system_dirs(State *s, ocfs2_filesys *fs);
>>>    static void create_lost_found_dir(State *s, ocfs2_filesys *fs);
>>>    static void format_journals(State *s, ocfs2_filesys *fs);
>>> @@ -823,6 +826,16 @@ main(int argc, char **argv)
>>>        if (!s->quiet)
>>>            printf("%s successful\n\n", s->progname);
>>>    +    free_directory(root_dir);
>>> +    free_directory(system_dir);
>>> +    for (i = 0; i < s->initial_slots; i++)
>>> +        free_directory(orphan_dir[i]);
>>> +
>>> +    for (i = 0; i < NUM_SYSTEM_INODES; i++)
>>> +        ocfs2_free(&record[i]);
>>> +
>>> +    free_state(s);
>>> +
>>>        return 0;
>>>    }
>>>    @@ -1227,6 +1240,25 @@ get_state(int argc, char **argv)
>>>        return s;
>>>    }
>>>    +static void
>>> +free_state(State *s)
>>> +{
>>> +    int i;
>>> +
>>> +    ocfs2_free(&s->vol_label);
>>> +    ocfs2_free(&s->device_name);
>>> +    ocfs2_free(&s->cluster_stack);
>>> +    ocfs2_free(&s->cluster_name);
>> Possibly, s->progname may hit being assigned strdup("mkfs.ocfs2").
>>
> Yes, s->progname need to be freed in this case.
>>> +
>>> +    for (i = 0; i < s->nr_cluster_groups; i++)
>>> +        free_alloc_group(s->global_bm->groups[i]);
>>> +    ocfs2_free(&s->global_bm->groups);
>>> +    ocfs2_free(&s->global_bm->name);
>>> +     ocfs2_free(&s->global_bm);
>> This seems to be the complex part;-) So, how about
>> _AllocBitmp::bm_record,
>> _AllocBitmp::alloc_record,
>> _AllocGroup::alloc_inode?
>>
>> TBH, i cannot figure this out clearly;-)
>>
>> Thanks,
>> Eric
>>
> _AllocBitmp::bm_record is assigned with an element of
> 'record[NUM_SYSTEM_INODES]', and 'record' will be freed at the end of
> main() described below:
> tmprec = &(record[GLOBAL_INODE_ALLOC_SYSTEM_INODE][0]);
> initialize_bitmap (s, s->volume_size_in_clusters,
> 		s->cluster_size_bits,
> 		"global bitmap", tmprec);
> bitmap->bm_record = bm_record;
>
> _AllocGroup::alloc_inode is also an element of
> 'record[NUM_SYSTEM_INODES]' described below:
> tmprec = &(record[GLOBAL_INODE_ALLOC_SYSTEM_INODE][0]);
> initialize_alloc_group(s, "system inode group", tmprec,
> 		crap_rec.extent_off >> s->blocksize_bits,
> 		0,
> 		crap_rec.extent_len >> s->cluster_size_bits,
> 		s->cluster_size / s->blocksize);
> group->alloc_inode = alloc_inode;
>
> _AllocBitmp::alloc_record is not used yet.

You've cleared my doubts. Thanks a lot for your clarification!

Eric

>
>>> +
>>> +    free_alloc_group(s->system_group);
>>> +}
>>> +
>>>    static int
>>>    get_number(char *arg, uint64_t *res)
>>>    {
>>> @@ -1841,6 +1873,14 @@ initialize_alloc_group(State *s, const char *name,
>>>        return group;
>>>    }
>>>    +static void
>>> +free_alloc_group(AllocGroup *group)
>>> +{
>>> +    ocfs2_free(&group->name);
>>> +    ocfs2_free(&group->gd);
>>> +    ocfs2_free(&group);
>>> +}
>>> +
>>>    static AllocBitmap *
>>>    initialize_bitmap(State *s, uint32_t bits, uint32_t unit_bits,
>>>              const char *name, SystemFileDiskRecord *bm_record)
>>> @@ -2129,6 +2169,13 @@ alloc_directory(State *s)
>>>    }
>>>      static void
>>> +free_directory(DirData *dir)
>>> +{
>>> +    ocfs2_free(&dir->buf);
>>> +    ocfs2_free(&dir);
>>> +}
>>> +
>>> +static void
>>>    add_entry_to_directory(State *s, DirData *dir, char *name, uint64_t byte_off,
>>>                   uint8_t type)
>>>    {
>>
>>
>> .
>>
>