modpost: release allocation when early return no suffix .o in read_symbols()
PATCH 1/1 modpost: release allocation when early return no suffix .o in read_symbols()
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".
--- scripts/mod/modpost.c | 1 + 1 file changed, 1 insertion(+)
scripts/mod/modpost.c +1 -0
@@ -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
Signature · 1 line
2.54.0
Nathan Chancellor
maintainerscripts/mod/modpost.c static void read_symbols(const char *modname) · line 1591
Quoted 12 lines · Robertus Diawan Chris
scripts/mod/modpost.c +1
-0@@ -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.
Signature · 2 lines
Cheers, Nathan
Hello Nathan,
Quoted 7 lines · Nathan Chancellor
Quoted 6 lines · Robertus Diawan Chris
scripts/mod/modpost.c +0
-0@@ -1585,6 +1585,7 @@ static void read_symbols(const char *modname)
Quoted 10 lines
scripts/mod/modpost.c static void read_symbols(const char *modname) · line 1591
Quoted 6 lines
scripts/mod/modpost.c +1
-0if (!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 theparse_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().
Quoted 3 lines
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.
Quoted 3 lines
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
Nathan Chancellor
maintainerHi Robertus,
Quoted 19 lines · Robertus Diawan Chris
Quoted 7 lines · Nathan Chancellor
Quoted 6 lines · Robertus Diawan Chris
scripts/mod/modpost.c +0
-0@@ -1585,6 +1585,7 @@ static void read_symbols(const char *modname)Quoted 10 lines
scripts/mod/modpost.c static void read_symbols(const char *modname) · line 1591
Quoted 6 lines
scripts/mod/modpost.c +1
-0if (!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 theparse_elf()one resolve this as well?
Quoted 6 lines
Yes, moving the
strends()if block before theparse_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 likestrends()if block and theparse_elf()didn't depends on each other, but just in case I missed something, I decided not to change the execution order and just addparse_elf_finish().
Yeah, I can definitely see why you went the route that you did.
Quoted 6 lines
Quoted 3 lines
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!
Quoted 8 lines
Quoted 3 lines
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 beforeparse_elf()inread_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.
Signature · 2 lines
Cheers, Nathan