dangling pointer in so_list

dangling pointer in so_list

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

Hello,

I run into a gdb crash examining a core file. This happened on gdb 7.3, 
on QNX. Unfortunately, I could not come up with a reproducible testcase 
on gnu/linux due to differences in dynamic linkers, but offer a detailed 
explanation instead:

What happened is that a process loaded the same shared object more than 
once. Then it crashed and a core was generated.

In the core, we had a link map specifying the same shared object more 
than once. While traversing the link map, gdb loaded shared objects 
(symbols), thus associating each so_list object with an objfile object. 
During the process, it detected duplicates and associated multiple 
so_list objects with the same objfile instance.

At this point, a change to solib-search-path causes gdb to reload 
symbols, and the crash happens: while traversing so_list in 
solib.c:reload_shared_libraries_1, in one iteration gdb calls 
'free_objfile' with a pointer to an instance of the objfile. In a 
subsequent iteration, it tries to do the same with, now, dangling 
pointer to the same objfile object. Not good.

The attached patch fixes the issue.


Thanks,

Aleksandar Ristovski
QNX Software Systems



ChangeLog:
<date>  Aleksandar Ristovski  <aristovski@qnx.com>

         * solib.c (reload_shared_libraries_1): Check whether objfile is 
used before
         freeing it.

[-- Attachment #2: dangling_objfile_in_so_list-201108311551.patch --]
[-- Type: text/x-patch, Size: 878 bytes --]

Index: gdb/solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.153
diff -u -p -r1.153 solib.c
--- gdb/solib.c	30 Aug 2011 02:48:05 -0000	1.153
+++ gdb/solib.c	31 Aug 2011 19:20:44 -0000
@@ -1226,7 +1226,22 @@ reload_shared_libraries_1 (int from_tty)
 	      && filename_cmp (found_pathname, so->so_name) != 0))
 	{
 	  if (so->objfile && ! (so->objfile->flags & OBJF_USERLOADED))
-	    free_objfile (so->objfile);
+	    {
+	      struct so_list *pivot;
+	      int used = 0;
+
+	      for (pivot = so_list_head; pivot != NULL; pivot = pivot->next)
+		{
+		  if (pivot != so && pivot->objfile == so->objfile)
+		    {
+		      used = 1;
+		      break;
+		    }
+		}
+
+	      if (!used)
+		free_objfile (so->objfile);
+	    }
 	  remove_target_sections (so->abfd);
 	  free_so_symbols (so);
 	}

Re: dangling pointer in so_list

[Aleksandar Ristovski]

I should have mentioned that there were no regressions (tested on 
x86_64-unknown-linux-gnu configuration).

On 11-08-31 04:01 PM, Aleksandar Ristovski wrote:
> Hello,
>
> I run into a gdb crash examining a core file. This happened on gdb 7.3,
> on QNX. Unfortunately, I could not come up with a reproducible testcase
> on gnu/linux due to differences in dynamic linkers, but offer a detailed
> explanation instead:
>
> What happened is that a process loaded the same shared object more than
> once. Then it crashed and a core was generated.
>
> In the core, we had a link map specifying the same shared object more
> than once. While traversing the link map, gdb loaded shared objects
> (symbols), thus associating each so_list object with an objfile object.
> During the process, it detected duplicates and associated multiple
> so_list objects with the same objfile instance.
>
> At this point, a change to solib-search-path causes gdb to reload
> symbols, and the crash happens: while traversing so_list in
> solib.c:reload_shared_libraries_1, in one iteration gdb calls
> 'free_objfile' with a pointer to an instance of the objfile. In a
> subsequent iteration, it tries to do the same with, now, dangling
> pointer to the same objfile object. Not good.
>
> The attached patch fixes the issue.
>
>
> Thanks,
>
> Aleksandar Ristovski
> QNX Software Systems
>
>
>
> ChangeLog:
> <date> Aleksandar Ristovski <aristovski@qnx.com>
>
> * solib.c (reload_shared_libraries_1): Check whether objfile is used before
> freeing it.

Re: dangling pointer in so_list

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

It turns out that there is another function with almost identical loop. 
This time, in 'update_solib_list' gdb would do the same: in one 
iteration it frees an objfile, in another tries to free it again.

The attached patch supersedes the first one and extracts the logic for 
determining duplicates into a function, then uses the function.

Still no regressions.


Comments appreciated.



Thanks,

Aleksandar Ristovski
QNX Software Systems




ChangeLog has changed slightly:

<date>  Aleksandar Ristovski  <aristovski@qnx.com>

         * solib.c (used): New function.
         (update_solib_list, reload_shared_libraries_1): Check if 
objfile is used
         by another so_list object before freeing it.


[-- Attachment #2: dangling_objfile_in_so_list-201109011545.patch --]
[-- Type: text/x-patch, Size: 1760 bytes --]

Index: gdb/solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.153
diff -u -p -r1.153 solib.c
--- gdb/solib.c	30 Aug 2011 02:48:05 -0000	1.153
+++ gdb/solib.c	1 Sep 2011 19:56:37 -0000
@@ -633,6 +633,23 @@ solib_read_symbols (struct so_list *so, 
   return 0;
 }
 
+/* Return 1 if KNOWN->objfile is used by any other so_list object in the
+   HEAD list.  Return 0 otherwise.  */
+
+static int
+used (const struct so_list *const known, const struct so_list *const head)
+{
+  const struct so_list *pivot;
+  int found = 0;
+
+  for (pivot = head; pivot != NULL && !found; pivot = pivot->next)
+    {
+      if (pivot != known && pivot->objfile == known->objfile)
+	found = 1;
+    }
+  return found;
+}
+
 /* Synchronize GDB's shared object list with inferior's.
 
    Extract the list of currently loaded shared objects from the
@@ -749,7 +766,8 @@ update_solib_list (int from_tty, struct 
 	  *gdb_link = gdb->next;
 
 	  /* Unless the user loaded it explicitly, free SO's objfile.  */
-	  if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED))
+	  if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED)
+	      && !used (gdb, so_list_head))
 	    free_objfile (gdb->objfile);
 
 	  /* Some targets' section tables might be referring to
@@ -1225,7 +1243,8 @@ reload_shared_libraries_1 (int from_tty)
 	  || (found_pathname != NULL
 	      && filename_cmp (found_pathname, so->so_name) != 0))
 	{
-	  if (so->objfile && ! (so->objfile->flags & OBJF_USERLOADED))
+	  if (so->objfile && ! (so->objfile->flags & OBJF_USERLOADED)
+	      && !used (so, so_list_head))
 	    free_objfile (so->objfile);
 	  remove_target_sections (so->abfd);
 	  free_so_symbols (so);

Re: dangling pointer in so_list

[Jan Kratochvil]

On Fri, 02 Sep 2011 15:06:34 +0200, Aleksandar Ristovski wrote:
> --- gdb/solib.c	30 Aug 2011 02:48:05 -0000	1.153
> +++ gdb/solib.c	1 Sep 2011 19:56:37 -0000
> @@ -633,6 +633,23 @@ solib_read_symbols (struct so_list *so, 
>    return 0;
>  }
>  
> +/* Return 1 if KNOWN->objfile is used by any other so_list object in the
> +   HEAD list.  Return 0 otherwise.  */
> +
> +static int
> +used (const struct so_list *const known, const struct so_list *const head)
> +{
> +  const struct so_list *pivot;
> +  int found = 0;
> +
> +  for (pivot = head; pivot != NULL && !found; pivot = pivot->next)
> +    {
> +      if (pivot != known && pivot->objfile == known->objfile)
> +	found = 1;
> +    }
> +  return found;
> +}

static int
solist_used (const struct so_list *const known)
{
  const struct so_list *so;

  for (so = so_list_head; so != NULL; so = so->next)
    if (so != known && so->objfile == known->objfile)
      return 1;

  return 0;
}
(untested for consts)

I find both the `head' (we are on solib.c) and `found' variables needless
there.

Use some solib-identifying name (such as the common solib_ prefix).

OK without the `head' parameter and with the solib_ prefix.

GNU/Linux compatible testcase is probably not possible, one would need two
loaded solibs with the same vaddr of their `.text' section.


Thanks,
Jan

Re: dangling pointer in so_list

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On 11-09-02 04:17 PM, Jan Kratochvil wrote:
>
> Use some solib-identifying name (such as the common solib_ prefix).
>
> OK without the `head' parameter and with the solib_ prefix.
>

I have committed this now with the solib_ prefix and body of the 
function as suggested.

Attached is the final patch/ChangeLog for reference.


Thank you,

Aleksandar Ristovski
QNX Software Systems



ChangeLog:
     * solib.c (solib_used): New function.
     (update_solib_list, reload_shared_libraries_1): Check if objfile is 
used
     by another so_list object before freeing it.

[-- Attachment #2: dangling_objfile_in_so_list-201109121421.patch --]
[-- Type: text/x-patch, Size: 1689 bytes --]

Index: gdb/solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.153
diff -u -p -r1.153 solib.c
--- gdb/solib.c	30 Aug 2011 02:48:05 -0000	1.153
+++ gdb/solib.c	12 Sep 2011 18:21:42 -0000
@@ -633,6 +633,20 @@ solib_read_symbols (struct so_list *so, 
   return 0;
 }
 
+/* Return 1 if KNOWN->objfile is used by any other so_list object in the
+   SO_LIST_HEAD list.  Return 0 otherwise.  */
+
+static int
+solib_used (const struct so_list *const known)
+{
+  const struct so_list *pivot;
+
+  for (pivot = so_list_head; pivot != NULL; pivot = pivot->next)
+    if (pivot != known && pivot->objfile == known->objfile)
+      return 1;
+  return 0;
+}
+
 /* Synchronize GDB's shared object list with inferior's.
 
    Extract the list of currently loaded shared objects from the
@@ -749,7 +763,8 @@ update_solib_list (int from_tty, struct 
 	  *gdb_link = gdb->next;
 
 	  /* Unless the user loaded it explicitly, free SO's objfile.  */
-	  if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED))
+	  if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED)
+	      && !solib_used (gdb))
 	    free_objfile (gdb->objfile);
 
 	  /* Some targets' section tables might be referring to
@@ -1225,7 +1240,8 @@ reload_shared_libraries_1 (int from_tty)
 	  || (found_pathname != NULL
 	      && filename_cmp (found_pathname, so->so_name) != 0))
 	{
-	  if (so->objfile && ! (so->objfile->flags & OBJF_USERLOADED))
+	  if (so->objfile && ! (so->objfile->flags & OBJF_USERLOADED)
+	      && !solib_used (so))
 	    free_objfile (so->objfile);
 	  remove_target_sections (so->abfd);
 	  free_so_symbols (so);