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.
As for reference I meant vfs_get_super() likewise, it calls
fill_super() if .s_root is NULL.
Thanks,
Gao Xiang
