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

[Gao Xiang]

On 2025/9/9 18:01, Yuezhang.Mo at 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.Mo at sony.com>
>>> Reviewed-by: Friendy Su <friendy.su at sony.com>
>>> Reviewed-by: Daniel Palmer <daniel.palmer at 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.

As for reference I meant vfs_get_super() likewise, it calls
fill_super() if .s_root is NULL.

Thanks,
Gao Xiang