Re: [patch] nto target: fix null pointer dereference

Re: [patch] nto target: fix null pointer dereference

[Ulrich Weigand]

Aleksandar Ristovski wrote:
> Ulrich Weigand wrote:
> > Can you explain what the situation is that leads to a NULL
> > pointer here?
> 
> Your question made me go through the issue again.
> 
> To create inferior, we use spawnp. spawnp will do something like mmap
> the binary and that's pretty much it (DT_DEBUG in .dynamic will contain
> NULL pointer).
> 
> In procfs_create_inferior we call solib_create_inferior_hook, which will
> end up trying to determine loader base by reading inferior's memory at
> DT_DEBUG pointer. In our case, before the executable actually started
> executing, it will successfully read 0s, because dynamic loader has not
> been invoked yet, and the pointer to r_debug structure is not initialized.

I see.  However, so->lm_info->lm should still be always allocated by the
common solib-svr4.c routines (see svr4_current_sos):

      new->lm_info->l_addr = (CORE_ADDR)-1;
      new->lm_info->lm_addr = lm;
      new->lm_info->lm = xzalloc (lmo->link_map_size);

The only case where it is not allocated is in svr4_default_sos (which I
guess can happen in your case if the loader base is not found).

However, there the comment says:

      /* Nothing will ever check the cached copy of the link
         map if we set l_addr.  */
      new->lm_info->l_addr = debug_loader_offset;
      new->lm_info->lm_addr = 0;
      new->lm_info->lm = NULL;

Note the assumption that <lm> is only every used if <l_addr> is
not equal to -1.  This is also what the solib-svr4.c implementation
of LM_ADDR_CHECK does.

It seems the main problem is that NTO at some time copied some of
the logic from solib-svr4.c, but has not adapted to the changes that
were added to that file later on.  Not even the definition of struct
lm_info matches any more!

Maybe a more correct fix would be to mimic the new solib-svr4.c logic
and use the l_addr field to cache the load address?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] nto target: fix null pointer dereference

[Aleksandar Ristovski]

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

Ulrich Weigand wrote:
> 
> It seems the main problem is that NTO at some time copied some of
> the logic from solib-svr4.c, but has not adapted to the changes that
> were added to that file later on.  Not even the definition of struct
> lm_info matches any more!
> 
> Maybe a more correct fix would be to mimic the new solib-svr4.c logic
> and use the l_addr field to cache the load address?
> 

You are right. This is what I did in the new patch (attached).

Thanks,

Aleksandar

[-- Attachment #2: nto-tdep.c.200808131121.diff.ChangeLog --]
[-- Type: text/plain, Size: 193 bytes --]

2008-08-13  Aleksandar Ristovski  <aristovski@qnx.com>

	* nto-tdep.c (lm_info): Updated struct lm_info definition from
	solib-svr4.c
	(LM_ADDR): Use l_addr if available; if not, use link map.

[-- Attachment #3: nto-tdep.c.200808131121.diff --]
[-- Type: text/plain, Size: 1969 bytes --]

Index: gdb/nto-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/nto-tdep.c,v
retrieving revision 1.25
diff -u -p -r1.25 nto-tdep.c
--- gdb/nto-tdep.c	23 Jul 2008 13:36:00 -0000	1.25
+++ gdb/nto-tdep.c	13 Aug 2008 15:21:20 -0000
@@ -253,21 +253,39 @@ nto_parse_redirection (char *pargv[], co
    solib-svr4.c to support nto_relocate_section_addresses
    which is different from the svr4 version.  */
 
+/* Link map info to include in an allocated so_list entry */
+
 struct lm_info
-{
-  /* Pointer to copy of link map from inferior.  The type is char *
-     rather than void *, so that we may use byte offsets to find the
-     various fields without the need for a cast.  */
-  char *lm;
-};
+  {
+    /* Pointer to copy of link map from inferior.  The type is char *
+       rather than void *, so that we may use byte offsets to find the
+       various fields without the need for a cast.  */
+    gdb_byte *lm;
+
+    /* Amount by which addresses in the binary should be relocated to
+       match the inferior.  This could most often be taken directly
+       from lm, but when prelinking is involved and the prelink base
+       address changes, we may need a different offset, we want to
+       warn about the difference and compute it only once.  */
+    CORE_ADDR l_addr;
+
+    /* The target location of lm.  */
+    CORE_ADDR lm_addr;
+  };
+
 
 static CORE_ADDR
 LM_ADDR (struct so_list *so)
 {
-  struct link_map_offsets *lmo = nto_fetch_link_map_offsets ();
+  if (so->lm_info->l_addr == (CORE_ADDR)-1)
+    {
+      struct link_map_offsets *lmo = nto_fetch_link_map_offsets ();
 
-  return extract_typed_address (so->lm_info->lm + lmo->l_addr_offset,
-                                builtin_type_void_data_ptr);
+      return extract_typed_address (so->lm_info->lm + lmo->l_addr_offset,
+				    builtin_type_void_data_ptr);
+    }
+  else
+    return so->lm_info->l_addr;
 }
 
 static CORE_ADDR

Re: [patch] nto target: fix null pointer dereference

[Aleksandar Ristovski]

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

Ulrich Weigand wrote:
> 
> ... you might want to cache the address you've computed once


Changed accordingly, new patch attached.

Thanks,

Aleksandar


[-- Attachment #2: nto-tdep.c.200808141635.diff.ChangeLog --]
[-- Type: text/plain, Size: 208 bytes --]

2008-08-14  Aleksandar Ristovski  <aristovski@qnx.com>

	* nto-tdep.c (lm_info): Updated struct lm_info definition from
	solib-svr4.c
	(LM_ADDR): Use l_addr if available; if not, use link map and set l_addr.

[-- Attachment #3: nto-tdep.c.200808141635.diff --]
[-- Type: text/plain, Size: 1979 bytes --]

Index: gdb/nto-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/nto-tdep.c,v
retrieving revision 1.25
diff -u -p -r1.25 nto-tdep.c
--- gdb/nto-tdep.c	23 Jul 2008 13:36:00 -0000	1.25
+++ gdb/nto-tdep.c	14 Aug 2008 20:37:20 -0000
@@ -253,21 +253,39 @@ nto_parse_redirection (char *pargv[], co
    solib-svr4.c to support nto_relocate_section_addresses
    which is different from the svr4 version.  */
 
+/* Link map info to include in an allocated so_list entry */
+
 struct lm_info
-{
-  /* Pointer to copy of link map from inferior.  The type is char *
-     rather than void *, so that we may use byte offsets to find the
-     various fields without the need for a cast.  */
-  char *lm;
-};
+  {
+    /* Pointer to copy of link map from inferior.  The type is char *
+       rather than void *, so that we may use byte offsets to find the
+       various fields without the need for a cast.  */
+    gdb_byte *lm;
+
+    /* Amount by which addresses in the binary should be relocated to
+       match the inferior.  This could most often be taken directly
+       from lm, but when prelinking is involved and the prelink base
+       address changes, we may need a different offset, we want to
+       warn about the difference and compute it only once.  */
+    CORE_ADDR l_addr;
+
+    /* The target location of lm.  */
+    CORE_ADDR lm_addr;
+  };
+
 
 static CORE_ADDR
 LM_ADDR (struct so_list *so)
 {
-  struct link_map_offsets *lmo = nto_fetch_link_map_offsets ();
+  if (so->lm_info->l_addr == (CORE_ADDR)-1)
+    {
+      struct link_map_offsets *lmo = nto_fetch_link_map_offsets ();
 
-  return extract_typed_address (so->lm_info->lm + lmo->l_addr_offset,
-                                builtin_type_void_data_ptr);
+      so->lm_info->l_addr =
+	    extract_typed_address (so->lm_info->lm + lmo->l_addr_offset,
+				   builtin_type_void_data_ptr);
+    }
+  return so->lm_info->l_addr;
 }
 
 static CORE_ADDR

Re: [patch] nto target: fix null pointer dereference

[Ulrich Weigand]

Aleksandar Ristovski wrote:

>2008-08-14  Aleksandar Ristovski  <aristovski@qnx.com>
>
>	* nto-tdep.c (lm_info): Updated struct lm_info definition from
>	solib-svr4.c
>	(LM_ADDR): Use l_addr if available; if not, use link map and set l_addr.

This is OK, thanks!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] nto target: fix null pointer dereference

[Ulrich Weigand]

Aleksandar Ristovski wrote:

>diff -u -p -r1.25 nto-tdep.c
>--- gdb/nto-tdep.c	23 Jul 2008 13:36:00 -0000	1.25
>+++ gdb/nto-tdep.c	12 Aug 2008 16:33:18 -0000
>@@ -266,6 +266,8 @@ LM_ADDR (struct so_list *so)
> {
>   struct link_map_offsets *lmo = nto_fetch_link_map_offsets ();
> 
>+  if (so->lm_info->lm == NULL)
>+	return 0;
>   return extract_typed_address (so->lm_info->lm + lmo->l_addr_offset,
>                                 builtin_type_void_data_ptr);
> }

I'm wondering how this can ever be NULL ... I understand nto
reuses the solib-svr4.c version of current_sos, which seems
to always initalize the lm member.

Can you explain what the situation is that leads to a NULL
pointer here?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] nto target: fix null pointer dereference

[Aleksandar Ristovski]

Ulrich Weigand wrote:
> Aleksandar Ristovski wrote:
> 
>> diff -u -p -r1.25 nto-tdep.c
>> --- gdb/nto-tdep.c	23 Jul 2008 13:36:00 -0000	1.25
>> +++ gdb/nto-tdep.c	12 Aug 2008 16:33:18 -0000
>> @@ -266,6 +266,8 @@ LM_ADDR (struct so_list *so)
>> {
>>   struct link_map_offsets *lmo = nto_fetch_link_map_offsets ();
>>
>> +  if (so->lm_info->lm == NULL)
>> +	return 0;
>>   return extract_typed_address (so->lm_info->lm + lmo->l_addr_offset,
>>                                 builtin_type_void_data_ptr);
>> }
> 
> I'm wondering how this can ever be NULL ... I understand nto
> reuses the solib-svr4.c version of current_sos, which seems
> to always initalize the lm member.
> 
> Can you explain what the situation is that leads to a NULL
> pointer here?
> 


Your question made me go through the issue again.

To create inferior, we use spawnp. spawnp will do something like mmap the binary and that's pretty much it (DT_DEBUG in .dynamic will contain NULL pointer).

in procfs_create_inferior we call solib_create_inferior_hook, which will end up trying to determine loader base by reading inferior's memory at DT_DEBUG pointer. In our case, before the executable actually started executing, it will successfully read 0s, because dynamic loader  has not been invoked yet, and the pointer to r_debug structure is not initialized.


Thanks,

Aleksandar

Re: [patch] nto target: fix null pointer dereference

[Daniel Jacobowitz]

On Tue, Aug 12, 2008 at 05:25:40PM -0400, Aleksandar Ristovski wrote:
> in procfs_create_inferior we call solib_create_inferior_hook, which will end up trying to determine loader base by reading inferior's memory at DT_DEBUG pointer. In our case, before the executable actually started executing, it will successfully read 0s, because dynamic loader  has not been invoked yet, and the pointer to r_debug structure is not initialized.

It should already detect that zero is invalid, since that's the same
value used initially; see e.g. calls to locate_base.  Where's the read
come from?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] nto target: fix null pointer dereference

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Tue, Aug 12, 2008 at 05:25:40PM -0400, Aleksandar Ristovski wrote:
>> in procfs_create_inferior we call solib_create_inferior_hook, which will end up trying to determine loader base by reading inferior's memory at DT_DEBUG pointer. In our case, before the executable actually started executing, it will successfully read 0s, because dynamic loader  has not been invoked yet, and the pointer to r_debug structure is not initialized.
> 
> It should already detect that zero is invalid, since that's the same
> value used initially; see e.g. calls to locate_base.  Where's the read
> come from?
> 
nto_procfs.c::LM_ADDR gets called only by nto_relocate_section_addresses which replaces svr4_relocate_section_addresses. There is a slight difference in the way we treat l_addr address comparing to svr4.

I will repost modified patch shortly.

[patch] nto target: fix null pointer dereference

[Aleksandar Ristovski]

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

Hello,

Simple patch to fix null pointer dereference. This should make qnx target operational and usable. 

Thanks,

Aleksandar Ristovski
QNX Software Systems

[-- Attachment #2: nto-tdep.c-20080812.diff.ChangeLog --]
[-- Type: text/plain, Size: 136 bytes --]

2008-08-12  Aleksandar Ristovski  <aristovski@qnx.com>

	* nto-tdep.c (LM_ADDR): Make sure the address is valid before
	dereferencing.


[-- Attachment #3: nto-tdep.c-20080812.diff --]
[-- Type: text/plain, Size: 580 bytes --]

Index: gdb/nto-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/nto-tdep.c,v
retrieving revision 1.25
diff -u -p -r1.25 nto-tdep.c
--- gdb/nto-tdep.c	23 Jul 2008 13:36:00 -0000	1.25
+++ gdb/nto-tdep.c	12 Aug 2008 16:33:18 -0000
@@ -266,6 +266,8 @@ LM_ADDR (struct so_list *so)
 {
   struct link_map_offsets *lmo = nto_fetch_link_map_offsets ();
 
+  if (so->lm_info->lm == NULL)
+	return 0;
   return extract_typed_address (so->lm_info->lm + lmo->l_addr_offset,
                                 builtin_type_void_data_ptr);
 }