[PATCH] fs/erofs: Fix realloc error handling

[Tom Rini]

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 <fberder at outlook.fr>
> > > >> ---
> > > >>  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20251201/5a22cd2c/attachment.sig>