On 2025/9/10 14:07 Gao Xiang wrote:
> On 2025/9/10 14:03, yuezhang...@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...@sony.com>
>>>> Reviewed-by: Friendy Su <friendy...@sony.com>
>>>> Reviewed-by: Daniel Palmer <daniel.pal...@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`...
>
OK, I will introduce and use a new `sbi->sb_valid` boolean.
>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();
