On 2025/9/9 18:14 Gao Xiang wrote:
> On 2025/9/9 18:01, yuezhang...@sony.com wrote:
>> On 2025/9/9 15:26, Gao Xiang wrote:
>>> Hi Yuezhang,
>>>
>>> On 2025/9/9 10:52, 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. Fix memory leak by calling erofs_put_super().
>>>> In main(), erofs_read_superblock() is called only to get the block
>>>> size. In erofs_mkfs_rebuild_load_trees(), erofs_read_superblock()
>>>> is called again, causing sbi->devs to be overwritten without being
>>>> released.
>>>>
>>>> Signed-off-by: Yuezhang Mo <yuezhang...@sony.com>
>>>> Reviewed-by: Friendy Su <friendy...@sony.com>
>>>> Reviewed-by: Daniel Palmer <daniel.pal...@sony.com>
>>>
>>> Thanks for your patch and sorry for a bit delay...
>>>
>>>> ---
>>>> lib/compress.c | 2 ++
>>>> lib/inode.c | 3 +++
>>>> lib/rebuild.c | 2 +-
>>>> mkfs/main.c | 3 ++-
>>>> 4 files changed, 8 insertions(+), 2 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..0882875 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->chunkindexes)
>>>> + free(inode->chunkindexes);
>>>
>>> For now, this needs a check
>>>
>>> if (inode->datalayout == EROFS_INODE_CHUNK_BASED)
>>> free(inode->chunkindexes);
>>>
>>> I admit it's not very clean, but otherwise it doesn't work
>>> since `chunkindexes` is in a union.
>>>
>>
>> Okay, I will change to this check.
>>
>>>> free(inode);
>>>> return 0;
>>>> }
>>>> diff --git a/lib/rebuild.c b/lib/rebuild.c
>>>> index 0358567..18e5204 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;
>>>> diff --git a/mkfs/main.c b/mkfs/main.c
>>>> index 28588db..bcde787 100644
>>>> --- a/mkfs/main.c
>>>> +++ b/mkfs/main.c
>>>> @@ -1702,6 +1702,7 @@ int main(int argc, char **argv)
>>>> goto exit;
>>>> }
>>>> mkfs_blkszbits = src->blkszbits;
>>>> + erofs_put_super(src);
>>>
>>> Do you really need to fix this now (considering `mkfs.erofs`
>>> will exit finally)?
>>
>> As you said, mkfs.erofs will exit finally, it won't affect users
>> without this fix.
>>
>> The main purpose of this patch is to fix the memory allocation
>> issue in erofs_rebuild_write_blob_index(). It will cause a
>> segmentation fault if there are deduplications(If there are few
>> files, no segmentation fault occurs, but the files will be
>> inaccessible.).
>
> Yes, so I tend to not fix `erofs_put_super()` in this patch.
>
>>
>>>
>>> In priciple, I think it should be fixed with a reference and
>>> something similiar to the kernel fill_super.
>>>
>>> I'm not sure if you have more time to improve this anyway,
>>> but the current usage is not perfect on my side...
>>
>> The memory leak is caused by the erofs_sb_info of the first blob
>> device being initialized twice, how to fix with reference? I do not
>> get your point.
>
> I think we should skip erofs_read_superblock() if sbi is
> initialized at least.
I will skip erofs_read_superblock() if sbi->devs is not NULL.
>
> As for reference I meant vfs_get_super() likewise, it calls
> fill_super() if .s_root is NULL.
