[PATCH 5/5] rtl-shell.c: Resource leak (CID #1444140)

[Ryan Long]

CID 1444140: Resource leak in rtems_rtl_shell_object().

Closes #4300
---
 cpukit/libdl/rtl-shell.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cpukit/libdl/rtl-shell.c b/cpukit/libdl/rtl-shell.c
index 9f8a136..bcecdd4 100644
--- a/cpukit/libdl/rtl-shell.c
+++ b/cpukit/libdl/rtl-shell.c
@@ -733,14 +733,17 @@ rtems_rtl_shell_object (const rtems_printer* printer, int 
argc, char* argv[])
 if (dlinfo (RTLD_SELF, RTLD_DI_UNRESOLVED, &unresolved) < 0)
 {
   rtems_printf (printer, "error: %s: %s\n", argv[arg], dlerror ());
+  (void) dlclose (handle);
   return 1;
 }
 
 if (unresolved != 0)
 {
   rtems_printf (printer, "warning: unresolved symbols present\n");
+  (void) dlclose (handle);
   return 1;
 }
+  (void) dlclose (handle);
   }
   else if (strcmp (argv[arg], "unload") == 0)
   {
-- 
1.8.3.1

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 5/5] rtl-shell.c: Resource leak (CID #1444140)

[Chris Johns]

On 13/3/21 2:18 am, Ryan Long wrote:
> CID 1444140: Resource leak in rtems_rtl_shell_object().
> 
> Closes #4300
> ---
>  cpukit/libdl/rtl-shell.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/cpukit/libdl/rtl-shell.c b/cpukit/libdl/rtl-shell.c
> index 9f8a136..bcecdd4 100644
> --- a/cpukit/libdl/rtl-shell.c
> +++ b/cpukit/libdl/rtl-shell.c
> @@ -733,14 +733,17 @@ rtems_rtl_shell_object (const rtems_printer* printer, 
> int argc, char* argv[])
>  if (dlinfo (RTLD_SELF, RTLD_DI_UNRESOLVED, &unresolved) < 0)
>  {
>rtems_printf (printer, "error: %s: %s\n", argv[arg], dlerror ());
> +  (void) dlclose (handle);
>return 1;
>  }
>  
>  if (unresolved != 0)
>  {
>rtems_printf (printer, "warning: unresolved symbols present\n");
> +  (void) dlclose (handle);
>return 1;
>  }
> +  (void) dlclose (handle);
>}
>else if (strcmp (argv[arg], "unload") == 0)
>{
> 

The handle should be not closed in any of these cases. Have you run the command
after making these changes and played with command?

This shell command is a tool to load and play with loaded modules and one
command is to load a module and others let you play with it. The handle address
is printed on the console. Coverity may like to track and nag you about things
like this but it is not always right.

In relation to all these Coverity changes ... are all these changes being
tested? Is the boarder context of the change being examined in relation to the
specific change?

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 5/5] rtl-shell.c: Resource leak (CID #1444140)

[Gedare Bloom]

On Sun, Mar 14, 2021 at 8:27 PM Chris Johns  wrote:
>
> On 13/3/21 2:18 am, Ryan Long wrote:
> > CID 1444140: Resource leak in rtems_rtl_shell_object().
> >
> > Closes #4300
> > ---
> >  cpukit/libdl/rtl-shell.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/cpukit/libdl/rtl-shell.c b/cpukit/libdl/rtl-shell.c
> > index 9f8a136..bcecdd4 100644
> > --- a/cpukit/libdl/rtl-shell.c
> > +++ b/cpukit/libdl/rtl-shell.c
> > @@ -733,14 +733,17 @@ rtems_rtl_shell_object (const rtems_printer* printer, 
> > int argc, char* argv[])
> >  if (dlinfo (RTLD_SELF, RTLD_DI_UNRESOLVED, &unresolved) < 0)
> >  {
> >rtems_printf (printer, "error: %s: %s\n", argv[arg], dlerror ());
> > +  (void) dlclose (handle);
> >return 1;
> >  }
> >
> >  if (unresolved != 0)
> >  {
> >rtems_printf (printer, "warning: unresolved symbols present\n");
> > +  (void) dlclose (handle);
> >return 1;
> >  }
> > +  (void) dlclose (handle);
> >}
> >else if (strcmp (argv[arg], "unload") == 0)
> >{
> >
>
> The handle should be not closed in any of these cases. Have you run the 
> command
> after making these changes and played with command?
>
> This shell command is a tool to load and play with loaded modules and one
> command is to load a module and others let you play with it. The handle 
> address
> is printed on the console. Coverity may like to track and nag you about things
> like this but it is not always right.
>
> In relation to all these Coverity changes ... are all these changes being
> tested? Is the boarder context of the change being examined in relation to the
> specific change?
>

I think this is an important, and challenging, question, since some
things are not fully tested. In this case, we should identify/create
tests to exercise the undefined behavior first, and then commit a
change after testing. As the easy coverity issues are being fixed, now
we need to be more meticulous since we can't just "think through" the
modifications being proposed, such as this one (and the NULL out in
shell calls) without understanding the scope of possible invocations.
For example, maybe we need a test case for telnet shells, before we
make a change that impacts their potential useability.

> Chris
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel