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

[Yuezhang.Mo at sony.com]

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.).

>
> 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.