On Tue, Dec 02, 2025 at 02:40:03AM +0800, Gao Xiang wrote: > Hi, > > On Mon, Dec 01, 2025 at 10:55:02AM -0600, Tom Rini wrote: > > 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. > > > > Just saw this, actually this issue has been fixed in erofs-utils > commit a3a75f7af7b2 ("erofs-utils: misc: Fix potential memory leak > in realloc failure path") > > I think it's fine to merge this fix now, and the mid-term plan is to > sync the erofs-utils codebase again for recent features and bugfixes.
Sounds good to me! -- Tom
signature.asc
Description: PGP signature
