[PATCH] modpost: release allocation when early return no suffix .o in read_symbols()

[PATCH] modpost: release allocation when early return no suffix .o in read_symbols()

[Robertus Diawan Chris]

The allocation for elf info symsearch and hdr from parse_elf() haven't
been released when return because of modname didn't have suffix ".o".
So, release the allocation before early return because of modname
without suffix ".o".

This is reported by Coverity Scan as "Resource leak".

Fixes: 8c9ce89c5b63 ("modpost: simplify mod->name allocation")
Signed-off-by: Robertus Diawan Chris <robertusdchris@gmail.com>
---
 scripts/mod/modpost.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index abbcd3fc1394..8e231544f9f3 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1585,6 +1585,7 @@ static void read_symbols(const char *modname)
 
 	if (!strends(modname, ".o")) {
 		error("%s: filename must be suffixed with .o\n", modname);
+		parse_elf_finish(&info);
 		return;
 	}
 

base-commit: 4549871118cf616eecdd2d939f78e3b9e1dddc48
-- 
2.54.0

Re: [PATCH] modpost: release allocation when early return no suffix .o in read_symbols()

[Nathan Chancellor]

On Wed, 10 Jun 2026 12:25:50 +0700, Robertus Diawan Chris <robertusdchris@gmail.com> wrote:
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index abbcd3fc1394..8e231544f9f3 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1585,6 +1585,7 @@ static void read_symbols(const char *modname)
>  
>  	if (!strends(modname, ".o")) {
>  		error("%s: filename must be suffixed with .o\n", modname);
> +		parse_elf_finish(&info);
>  		return;
>  	}
>  

Thanks for the patch! While this change appears to be correct, would
moving the strends() if block before the parse_elf() one resolve this as
well? I think I would prefer going that route because neither check
really depends on the other and we have to think less about unwinding
with the checks flipped. Furthermore, modpost is a relatively short
running host utility, so I don't really think it is worth optimizing for
resource leaks like this.

-- 
Cheers,
Nathan

Re: [PATCH] modpost: release allocation when early return no suffix .o in read_symbols()

[Robertus Diawan Chris]

Hello Nathan,

On Tue, Jun 16, 2026 at 09:21:52PM -0700, Nathan Chancellor wrote:
> On Wed, 10 Jun 2026 12:25:50 +0700, Robertus Diawan Chris <robertusdchris@gmail.com> wrote:
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index abbcd3fc1394..8e231544f9f3 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1585,6 +1585,7 @@ static void read_symbols(const char *modname)
> >
> >  	if (!strends(modname, ".o")) {
> >  		error("%s: filename must be suffixed with .o\n", modname);
> > +		parse_elf_finish(&info);
> >  		return;
> >  	}
> >
>
> Thanks for the patch! While this change appears to be correct, would
> moving the strends() if block before the parse_elf() one resolve this as
> well?

Yes, moving the strends() if block before the parse_elf() also resolve
this too. The reason I didn't do that because I am still not sure if
changing the order will have any effect. I already take a look at the
code and it looks like strends() if block and the parse_elf() didn't
depends on each other, but just in case I missed something, I decided
not to change the execution order and just add parse_elf_finish().

> I think I would prefer going that route because neither check really
> depends on the other and we have to think less about unwinding
> with the checks flipped.

Now that you already confirm that neither check depends on each other, I
am more confident to take this approach.

> Furthermore, modpost is a relatively short running host utility, so
> I don't really think it is worth optimizing for resource leaks like
> this.

I want to confirm something first, do you mind if I send v2 patch with
the change of moving strends() if block before parse_elf() in
read_symbols()? I mean, if you feel like this is unnecessary, I will
drop this patch. I don't mind either way.

Thank you for the feedback.

Best regards,
Robertus Diawan Chris

Re: [PATCH] modpost: release allocation when early return no suffix .o in read_symbols()

[Nathan Chancellor]

Hi Robertus,

On Thu, Jun 18, 2026 at 04:46:35AM +0700, Robertus Diawan Chris wrote:
> On Tue, Jun 16, 2026 at 09:21:52PM -0700, Nathan Chancellor wrote:
> > On Wed, 10 Jun 2026 12:25:50 +0700, Robertus Diawan Chris <robertusdchris@gmail.com> wrote:
> > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > > index abbcd3fc1394..8e231544f9f3 100644
> > > --- a/scripts/mod/modpost.c
> > > +++ b/scripts/mod/modpost.c
> > > @@ -1585,6 +1585,7 @@ static void read_symbols(const char *modname)
> > >
> > >  	if (!strends(modname, ".o")) {
> > >  		error("%s: filename must be suffixed with .o\n", modname);
> > > +		parse_elf_finish(&info);
> > >  		return;
> > >  	}
> > >
> >
> > Thanks for the patch! While this change appears to be correct, would
> > moving the strends() if block before the parse_elf() one resolve this as
> > well?
> 
> Yes, moving the strends() if block before the parse_elf() also resolve
> this too. The reason I didn't do that because I am still not sure if
> changing the order will have any effect. I already take a look at the
> code and it looks like strends() if block and the parse_elf() didn't
> depends on each other, but just in case I missed something, I decided
> not to change the execution order and just add parse_elf_finish().

Yeah, I can definitely see why you went the route that you did.

> > I think I would prefer going that route because neither check really
> > depends on the other and we have to think less about unwinding
> > with the checks flipped.
> 
> Now that you already confirm that neither check depends on each other, I
> am more confident to take this approach.

Sounds good to me!

> > Furthermore, modpost is a relatively short running host utility, so
> > I don't really think it is worth optimizing for resource leaks like
> > this.
> 
> I want to confirm something first, do you mind if I send v2 patch with
> the change of moving strends() if block before parse_elf() in
> read_symbols()? I mean, if you feel like this is unnecessary, I will
> drop this patch. I don't mind either way.

I do not mind taking patches that silence static checker warnings when
they are either small or do not make the code much more complicated to
understand. I think this change would satisfy both of those points, so
feel free to send a v2. I just do not see these type of fixes as high
priority.

-- 
Cheers,
Nathan