[v2,(w/,ChangeLog)] Fix disabling of solib probes when LD_AUDITing

Message ID 1ehh33uyr5gra7_h9krnpznz2jqsjw_4593wafg.zlb1mgays0ef@mail.bob131.so
State New
Headers show
Series
  • [v2,(w/,ChangeLog)] Fix disabling of solib probes when LD_AUDITing
Related show

Commit Message

George Barrett Dec. 15, 2019, 12:12 a.m.
The SVR4 solib event handler determines whether an event is related to a
non-base link namespace by comparing the event's debug struct address
to the debug struct address of the initial program image. However, this
can fail when using LD_AUDIT as audit libraries are loaded before the
loader has initialised the initial program image's debug struct. When
the event handler fails to find the debug struct, the probe-based
debugger interface is disabled and a warning is flagged to the user.

This commit adds a fallback test to help determine whether an event is
for a foreign link namespace when the debug struct isn't available.

gdb/ChangeLog:
2019-12-15  George Barrett  <bob@bob131.so>

	* solib-svr4.c (svr4_handle_solib_event): Add fallback link
	namespace test for when the debug struct isn't available.
---
 gdb/solib-svr4.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

-- 
2.23.0

Comments

Simon Marchi Dec. 15, 2019, 2:30 a.m. | #1
On 2019-12-14 7:12 p.m., George Barrett wrote:
> The SVR4 solib event handler determines whether an event is related to a

> non-base link namespace by comparing the event's debug struct address

> to the debug struct address of the initial program image. However, this

> can fail when using LD_AUDIT as audit libraries are loaded before the

> loader has initialised the initial program image's debug struct. When

> the event handler fails to find the debug struct, the probe-based

> debugger interface is disabled and a warning is flagged to the user.

> 

> This commit adds a fallback test to help determine whether an event is

> for a foreign link namespace when the debug struct isn't available.


Hi George,

The patch makes sense to me, as far as I understand it.

> gdb/ChangeLog:

> 2019-12-15  George Barrett  <bob@bob131.so>

> 

> 	* solib-svr4.c (svr4_handle_solib_event): Add fallback link

> 	namespace test for when the debug struct isn't available.

> ---

>  gdb/solib-svr4.c | 22 +++++++++++++++++++++-

>  1 file changed, 21 insertions(+), 1 deletion(-)

> 

> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c

> index de765576d0..f0c7769ac2 100644

> --- a/gdb/solib-svr4.c

> +++ b/gdb/solib-svr4.c

> @@ -1942,7 +1942,27 @@ svr4_handle_solib_event (void)

>      /* Always locate the debug struct, in case it moved.  */

>      info->debug_base = 0;

>      if (locate_base (info) == 0)

> -      return;

> +      {

> +	/* It's possible for the reloc_complete probe to be triggered before

> +	   the linker has set the DT_DEBUG pointer (for example, when the

> +	   linker has finished relocating an LD_AUDIT library or its

> +	   dependencies).  Since we can't yet handle libraries from other link

> +	   namespaces, we don't lose anything by ignoring them here.  */

> +	struct value *link_map_id_val;

> +	try

> +	  {

> +	    link_map_id_val = pa->prob->evaluate_argument (0, frame);

> +	  }

> +	catch (const gdb_exception_error)


Catch the exception by reference:

  catch (const gdb_exception_error &)

I can push your patch with this fixed, if you agree (and others think the patch is
fine too).

Simon
George Barrett Dec. 15, 2019, 2:33 a.m. | #2
On Sat, Dec 14, 2019 at 09:30:43PM -0500, Simon Marchi wrote:
> Catch the exception by reference:

>

>   catch (const gdb_exception_error &)

>

> I can push your patch with this fixed, if you agree (and others think the

> patch is fine too).


Yes, that's fine by me.

Thanks
Simon Marchi Dec. 15, 2019, 2:34 a.m. | #3
On 2019-12-14 9:33 p.m., George Barrett wrote:
> On Sat, Dec 14, 2019 at 09:30:43PM -0500, Simon Marchi wrote:

>> Catch the exception by reference:

>>

>>   catch (const gdb_exception_error &)

>>

>> I can push your patch with this fixed, if you agree (and others think the

>> patch is fine too).

> 

> Yes, that's fine by me.

> 

> Thanks

> 


Ok, I'd like to leave a bit of time for others to comment, so please
ping in a week or so if there hasn't been any other activity, I'll merge
it then.

Simon
George Barrett Dec. 21, 2019, 3:25 p.m. | #4
On Sat, Dec 14, 2019 at 09:34:30PM -0500, Simon Marchi wrote:
> Ok, I'd like to leave a bit of time for others to comment, so please

> ping in a week or so if there hasn't been any other activity, I'll merge

> it then.


Pinging, as requested.
Simon Marchi Dec. 21, 2019, 9:03 p.m. | #5
On 2019-12-21 10:25 a.m., George Barrett wrote:
> On Sat, Dec 14, 2019 at 09:34:30PM -0500, Simon Marchi wrote:

>> Ok, I'd like to leave a bit of time for others to comment, so please

>> ping in a week or so if there hasn't been any other activity, I'll merge

>> it then.

> 

> Pinging, as requested.

> 


Thanks, I have pushed it.

Simon

Patch

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index de765576d0..f0c7769ac2 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1942,7 +1942,27 @@  svr4_handle_solib_event (void)
     /* Always locate the debug struct, in case it moved.  */
     info->debug_base = 0;
     if (locate_base (info) == 0)
-      return;
+      {
+	/* It's possible for the reloc_complete probe to be triggered before
+	   the linker has set the DT_DEBUG pointer (for example, when the
+	   linker has finished relocating an LD_AUDIT library or its
+	   dependencies).  Since we can't yet handle libraries from other link
+	   namespaces, we don't lose anything by ignoring them here.  */
+	struct value *link_map_id_val;
+	try
+	  {
+	    link_map_id_val = pa->prob->evaluate_argument (0, frame);
+	  }
+	catch (const gdb_exception_error)
+	  {
+	    link_map_id_val = NULL;
+	  }
+	/* glibc and illumos' libc both define LM_ID_BASE as zero.  */
+	if (link_map_id_val != NULL && value_as_long (link_map_id_val) != 0)
+	  action = DO_NOTHING;
+	else
+	  return;
+      }
 
     /* GDB does not currently support libraries loaded via dlmopen
        into namespaces other than the initial one.  We must ignore