[PATCH v2] erofs-utils: fix memory leaks and allocation issue

[Gao Xiang]

Hi Yuezhang,

On 2025/9/10 14:03, Yuezhang.Mo at sony.com wrote:
> On 2025/9/10 13:14 Gao Xiang wrote:
>> On 2025/9/10 13:05, Yuezhang Mo wrote:
>>> This patch addresses 4 memory leaks and 1 allocation issue to
>>> ensure proper cleanup and allocation:
>>>
>>> 1. Fixed memory leak by freeing sbi->zmgr in z_erofs_compress_exit().
>>> 2. Fixed memory leak by freeing inode->chunkindexes in erofs_iput().
>>> 3. Fixed incorrect allocation of chunk index array in
>>>      erofs_rebuild_write_blob_index() to ensure correct array allocation
>>>      and avoid out-of-bounds access.
>>> 4. Fixed memory leak of struct erofs_blobchunk not being freed by
>>>      calling erofs_blob_exit() in rebuild mode.
>>> 5. Fixed memory leak caused by repeated initialization of the first
>>>      blob device's sbi by checking whether sbi has been initialized.
>>>
>>> Signed-off-by: Yuezhang Mo <Yuezhang.Mo at sony.com>
>>> Reviewed-by: Friendy Su <friendy.su at sony.com>
>>> Reviewed-by: Daniel Palmer <daniel.palmer at sony.com>
>>> ---
>>>    lib/compress.c |  2 ++
>>>    lib/inode.c    |  3 +++
>>>    lib/rebuild.c  | 13 ++++++++-----
>>>    mkfs/main.c    |  2 +-
>>>    4 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/compress.c b/lib/compress.c
>>> index 622a205..dd537ec 100644
>>> --- a/lib/compress.c
>>> +++ b/lib/compress.c
>>> @@ -2171,5 +2171,7 @@ int z_erofs_compress_exit(struct erofs_sb_info *sbi)
>>>                }
>>>    #endif
>>>        }
>>> +
>>> +     free(sbi->zmgr);
>>>        return 0;
>>>    }
>>> diff --git a/lib/inode.c b/lib/inode.c
>>> index 7ee6b3d..38e2bb2 100644
>>> --- a/lib/inode.c
>>> +++ b/lib/inode.c
>>> @@ -159,6 +159,9 @@ unsigned int erofs_iput(struct erofs_inode *inode)
>>>        } else {
>>>                free(inode->i_link);
>>>        }
>>> +
>>> +     if (inode->datalayout == EROFS_INODE_CHUNK_BASED)
>>> +             free(inode->chunkindexes);
>>>        free(inode);
>>>        return 0;
>>>    }
>>> diff --git a/lib/rebuild.c b/lib/rebuild.c
>>> index 0358567..461e18e 100644
>>> --- a/lib/rebuild.c
>>> +++ b/lib/rebuild.c
>>> @@ -186,7 +186,7 @@ static int erofs_rebuild_write_blob_index(struct erofs_sb_info *dst_sb,
>>>
>>>        unit = sizeof(struct erofs_inode_chunk_index);
>>>        inode->extent_isize = count * unit;
>>> -     idx = malloc(max(sizeof(*idx), sizeof(void *)));
>>> +     idx = calloc(count, max(sizeof(*idx), sizeof(void *)));
>>>        if (!idx)
>>>                return -ENOMEM;
>>>        inode->chunkindexes = idx;
>>> @@ -428,10 +428,13 @@ int erofs_rebuild_load_tree(struct erofs_inode *root, struct erofs_sb_info *sbi,
>>>                erofs_uuid_unparse_lower(sbi->uuid, uuid_str);
>>>                fsid = uuid_str;
>>>        }
>>> -     ret = erofs_read_superblock(sbi);
>>> -     if (ret) {
>>> -             erofs_err("failed to read superblock of %s", fsid);
>>> -             return ret;
>>> +
>>> +     if (!sbi->devs) {
>>
>> `sbi->devs` may be NULL if ondisk_extradevs == 0? (see
>> erofs_init_devices()).
>>
>> I think we could just introduce a new `sbi->sb_valid`
>> boolean, and set up this in erofs_read_superblock()
>> instead in the short term.
> 
> For rebuild mode, ondisk_extradevs is not 0, `sbi->devs` is always set.

I think `ondisk_extradevs` may be 0 if `--blobdev` isn't used,
but we can still use rebuild mode, e.g.

	mkfs.erofs -Enoinline_data 1.erofs foo1
	mkfs.erofs -Enoinline_data 2.erofs foo2
	mkfs.erofs merge.erofs 1.erofs 2.erofs

> 
> Is there a case where erofs_read_superblock() is called multiple times
> if not in rebuild mode? Or will there be such a case in the future?

Apart from that, I think introducing a sb_valid boolean is cleaner
(although both are not perfect...) than reusing `sbi->devs`...

Thanks,
Gao Xiang

> 
>>
>> Thanks,
>> Gao Xiang
>>
>>> +             ret = erofs_read_superblock(sbi);
>>> +             if (ret) {
>>> +                     erofs_err("failed to read superblock of %s", fsid);
>>> +                     return ret;
>>> +             }
>>>        }
>>>
>>>        inode.nid = sbi->root_nid;
>>> diff --git a/mkfs/main.c b/mkfs/main.c
>>> index 28588db..3dd5815 100644
>>> --- a/mkfs/main.c
>>> +++ b/mkfs/main.c
>>> @@ -1908,7 +1908,7 @@ exit:
>>>        erofs_dev_close(&g_sbi);
>>>        erofs_cleanup_compress_hints();
>>>        erofs_cleanup_exclude_rules();
>>> -     if (cfg.c_chunkbits)
>>> +     if (cfg.c_chunkbits || source_mode == EROFS_MKFS_SOURCE_REBUILD)
>>>                erofs_blob_exit();
>>>        erofs_xattr_cleanup_name_prefixes();
>>>        erofs_rebuild_cleanup();