On Sat, Nov 29, 2025 at 12:12:23PM +0100, Francois Berder wrote: > Hello Tom, > > On 11/18/25 22:33, Tom Rini wrote: > > On Tue, Nov 11, 2025 at 01:49:30PM +0100, Francois Berder wrote: > > > >> If realloc failed, raw was not freed and thus memory > >> was leaked. > >> > >> Signed-off-by: Francois Berder <[email protected]> > >> --- > >> fs/erofs/data.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/erofs/data.c b/fs/erofs/data.c > >> index 95b609d8ea8..b58ec6fcc66 100644 > >> --- a/fs/erofs/data.c > >> +++ b/fs/erofs/data.c > >> @@ -319,12 +319,15 @@ static int z_erofs_read_data(struct erofs_inode > >> *inode, char *buffer, > >> } > >> > >> if (map.m_plen > bufsize) { > >> + char *tmp; > >> + > >> bufsize = map.m_plen; > >> - raw = realloc(raw, bufsize); > >> - if (!raw) { > >> + tmp = realloc(raw, bufsize); > >> + if (!tmp) { > >> ret = -ENOMEM; > >> break; > >> } > >> + raw = tmp; > >> } > >> > >> ret = z_erofs_read_one_data(inode, &map, raw, > > > > I'm not sure how this changes anything? The function is currently > > (snipped for clarity): > > static int z_erofs_read_data(struct erofs_inode *inode, char *buffer, > > erofs_off_t size, erofs_off_t offset) > > { > > [snip] > > char *raw = NULL; > > [snip] > > if (map.m_plen > bufsize) { > > bufsize = map.m_plen; > > raw = realloc(raw, bufsize); > > if (!raw) { > > ret = -ENOMEM; > > break; > > } > > } > > > > ret = z_erofs_read_one_data(inode, &map, raw, > > buffer + end - offset, skip, length, > > trimmed); > > if (ret < 0) > > break; > > } > > if (raw) > > free(raw); > > return ret < 0 ? ret : 0; > > } > > > > And per include/malloc.h, calling realloc with a null pointer is the > > same as calling malloc. So we had nothing previously allocated to free > > later when this failed. How did you find this particular issue? Thanks. > > > > I found this issue (and most bug fixes I submitted recently) by running > cppcheck and scan-build against u-boot source code and triaging warnings > reported by these tools. > > I think my fix should still be applied. Looking at z_erofs_map_blocks_iter > and z_erofs_do_map_blocks, I cannot guarantee that map.m_plen does not > increase during the loop execution. Hence, realloc could be called several > times and we need to properly handle realloc failure.
Thanks for explaining more. Yes, the loop part was what I missed on my earlier look over the code. -- Tom
signature.asc
Description: PGP signature
