diff mbox series

segment: Fix dangling pointer

Message ID 20240328202922.7025-1-maks.mishinFZ@gmail.com
State Rejected
Headers
Series segment: Fix dangling pointer |

Commit Message

Maks Mishin March 28, 2024, 8:29 p.m. UTC 
  Pointer 'lookup_module' which is a field of the structure 'Dwfl'
freed at segment.c:88 is not overwritten, but it is usually overwritten
after free.

Found by RASU JSC.

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 libdwfl/segment.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Mark Wielaard March 28, 2024, 9:04 p.m. UTC | #1 
Hi Maks,

On Thu, Mar 28, 2024 at 11:29:22PM +0300, Maks Mishin wrote:
> Pointer 'lookup_module' which is a field of the structure 'Dwfl'
> freed at segment.c:88 is not overwritten, but it is usually overwritten
> after free.

But the very next statement is a return true; so old isn't in scope
anymore. Why would we assign NULL to it?

> Found by RASU JSC.

What or who is that?

> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> ---
>  libdwfl/segment.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libdwfl/segment.c b/libdwfl/segment.c
> index f6a3e84e..af76f2f8 100644
> --- a/libdwfl/segment.c
> +++ b/libdwfl/segment.c
> @@ -86,6 +86,7 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start, GElf_Addr end, int segndx)
>  	  if (unlikely (dwfl->lookup_module == NULL))
>  	    {
>  	      free (old);
> +	      old = NULL;
>  	      return true;
>  	    }
>  	}
> -- 
> 2.30.2
>
Mark Wielaard April 3, 2024, 8:28 p.m. UTC | #2 
Hi Maks,

Adding the elfutils-devel list back to the CC because that is where
patches are discussed.

On Tue, Apr 02, 2024 at 10:45:59PM +0300, Максим Мишин wrote:
> RASU JSC is a part of Rosatom, a company where one of the areas of work is
> software development based on the Linux kernel.
> That's why I'm doing a static analysis of the Linux-core components.
> 
> We use the Svace static analyzer:
> https://www.ispras.ru/en/technologies/svace/
> 
> This patch is the processing of the analyzer's triggers with the
> DANGLING_POINTER type for pointer `old`.

OK, but that doesn't really make sense. old isn't a dangling
pointer. It is a local pointer that is freed before returning from the
function. What do you try to accomplish by assigning it the value
NULL?

What real issue are you trying to fix?

Thanks,

Mark

> пт, 29 мар. 2024 г. в 00:04, Mark Wielaard <mark@klomp.org>:
> 
> > Hi Maks,
> >
> > On Thu, Mar 28, 2024 at 11:29:22PM +0300, Maks Mishin wrote:
> > > Pointer 'lookup_module' which is a field of the structure 'Dwfl'
> > > freed at segment.c:88 is not overwritten, but it is usually overwritten
> > > after free.
> >
> > But the very next statement is a return true; so old isn't in scope
> > anymore. Why would we assign NULL to it?
> >
> > > Found by RASU JSC.
> >
> > What or who is that?
> >
> > > Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> > > ---
> > >  libdwfl/segment.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/libdwfl/segment.c b/libdwfl/segment.c
> > > index f6a3e84e..af76f2f8 100644
> > > --- a/libdwfl/segment.c
> > > +++ b/libdwfl/segment.c
> > > @@ -86,6 +86,7 @@ insert (Dwfl *dwfl, size_t i, GElf_Addr start,
> > GElf_Addr end, int segndx)
> > >         if (unlikely (dwfl->lookup_module == NULL))
> > >           {
> > >             free (old);
> > > +           old = NULL;
> > >             return true;
> > >           }
> > >       }
> > > --
> > > 2.30.2
> > >
> >
> 
> 
> -- 
> С уважением,
> Максим Мишин
> +7 (915) 958-41-07
> maks.mishinFZ@gmail.com
diff mbox series

Patch

diff --git a/libdwfl/segment.c b/libdwfl/segment.c
index f6a3e84e..af76f2f8 100644
--- a/libdwfl/segment.c
+++ b/libdwfl/segment.c
@@ -86,6 +86,7 @@  insert (Dwfl *dwfl, size_t i, GElf_Addr start, GElf_Addr end, int segndx)
 	  if (unlikely (dwfl->lookup_module == NULL))
 	    {
 	      free (old);
+	      old = NULL;
 	      return true;
 	    }
 	}