[PATCH] Skip VDSO when reading SO list

[PATCH] Skip VDSO when reading SO list

[Andreas Arnez]

On some Linux versions, the file name (l_name) of a VDSO's link map
entry lies in read-only memory that is excluded from a core dump.
When reading such a core dump, GDB complains:

warning: Can't read pathname for load map: Input/output error.

When GDB walks through the link map again after establishing the file
mappings, the file name will be recognized as the empty string, and
the entry will be ignored.  However, if the user supplied the wrong
version of the executable, a bogus entry with a garbage file name may
be created instead.

This patch tries to avoid the warning and the potential bogus entry.

In the cases I've looked at, the VDSO's l_name address matches that of
the main executable.  Thus the patch uses this as a criteria for
identifying (and skipping) the VDSO.


2013-08-19  Andreas Arnez  <arnez@linux.vnet.ibm.com>

	* solib-svr4.c (svr4_read_so_list): Skip the VDSO when reading
	link map entries.

Index: gdb/gdb/solib-svr4.c
===================================================================
--- gdb.orig/gdb/solib-svr4.c
+++ gdb/gdb/solib-svr4.c
@@ -1310,6 +1310,7 @@ static int
 svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
 		   struct so_list ***link_ptr_ptr, int ignore_first)
 {
+  struct so_list *first = NULL;
   CORE_ADDR next_lm;
 
   for (; lm != 0; prev_lm = lm, lm = next_lm)
@@ -1349,10 +1350,22 @@ svr4_read_so_list (CORE_ADDR lm, CORE_AD
 	{
 	  struct svr4_info *info = get_svr4_info ();
 
+	  first = new;
 	  info->main_lm_addr = new->lm_info->lm_addr;
 	  do_cleanups (old_chain);
 	  continue;
 	}
+
+      /* The l_name of a VDSO sometimes lies in read-only memory that
+	 is excluded from a core dump.  In order to avoid the "can't
+	 read pathname" warning, we try to identify the VDSO.  One
+	 criteria is that the l_name address matches that of the main
+	 executable.  */
+      if (first && new->lm_info->l_name == first->lm_info->l_name)
+	{
+	  do_cleanups (old_chain);
+	  continue;
+	}
 
       /* Extract this shared object's name.  */
       target_read_string (new->lm_info->l_name, &buffer,

Re: [PATCH] Skip VDSO when reading SO list

[Jan Kratochvil]

On Mon, 19 Aug 2013 16:44:13 +0200, Andreas Arnez wrote:
> On some Linux versions, the file name (l_name) of a VDSO's link map
> entry lies in read-only memory that is excluded from a core dump.
> When reading such a core dump, GDB complains:
> 
> warning: Can't read pathname for load map: Input/output error.

I believe the right fix is in glibc instead:
	[patch] Fix vDSO l_name for GDB's: Can't read pathname for load map:Input/output error.
	http://sourceware.org/ml/libc-alpha/2009-10/msg00001.html
	Message-ID: <20091004161706.GA27450@host0.dyn.jankratochvil.net>

Could you verify it works for you so that we could possibly ping it?

Fedora is using a similar patch but that is IMO a wrong workaround:
	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-core-open-vdso-warning.patch



Thanks,
Jan

Re: [PATCH] Skip VDSO when reading SO list

[Joel Brobecker]

> Fedora is using a similar patch but that is IMO a wrong workaround:
> 	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-core-open-vdso-warning.patch

If others can benefit from the patch, and the patch has no or little
adverse effect, I'd suggest putting the patch in the FSF tree,
especially since glib patches are not necessarily trivial to
install for the average user, not to mention the fact that the user
may not even have enough privileges to do so.

-- 
Joel

Re: [PATCH] Skip VDSO when reading SO list

[Jan Kratochvil]

On Mon, 19 Aug 2013 22:42:17 +0200, Joel Brobecker wrote:
> If others can benefit from the patch, and the patch has no or little
> adverse effect, I'd suggest putting the patch in the FSF tree,
> especially since glib patches are not necessarily trivial to
> install for the average user, not to mention the fact that the user
> may not even have enough privileges to do so.

With the GDB workaround in place it may be difficult to justify the glibc fix.
When glibc behaves correctly in the future then I think we can start talking
about GDB workarounds for older glibcs.

Another thing would be if you do not agree with the glibc patch; but that is
not the case here I think.


Jan

Re: [PATCH] Skip VDSO when reading SO list

[Joel Brobecker]

> With the GDB workaround in place it may be difficult to justify the
> glibc fix.  When glibc behaves correctly in the future then I think we
> can start talking about GDB workarounds for older glibcs.

I can understand your rationale. I've had situations like that,
at work. Unfortunately for me, I was always on the other side
of the fence, getting hurt so that the pressure could remain
on those whom we expected the fix from. It's a bit unfortunate,
but if you believe it is necessary, then so be it.

> Another thing would be if you do not agree with the glibc patch; but
> that is not the case here I think.

Not at all. The rationale is as you said: Although the fix will
hopefully eventually find itself in glibc, some users will still
be using older systems where the fix is missing. If we can help
those people (again, without hurting other users - or your chances
to get the glibc patch in), then let's do it.

-- 
Joel

Re: [PATCH] Skip VDSO when reading SO list

[Andreas Arnez]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> On Mon, 19 Aug 2013 16:44:13 +0200, Andreas Arnez wrote:
>> On some Linux versions, the file name (l_name) of a VDSO's link map
>> entry lies in read-only memory that is excluded from a core dump.
>> When reading such a core dump, GDB complains:
>> 
>> warning: Can't read pathname for load map: Input/output error.
>
> I believe the right fix is in glibc instead:
> 	[patch] Fix vDSO l_name for GDB's: Can't read pathname for load map:Input/output error.
> 	http://sourceware.org/ml/libc-alpha/2009-10/msg00001.html
> 	Message-ID: <20091004161706.GA27450@host0.dyn.jankratochvil.net>
>
> Could you verify it works for you so that we could possibly ping it?

This patch doesn't apply anymore, because the glibc code has been
restructured, e.g. the vdso logic is now separated out in
elf/setup-vdso.h.

Maybe the following patch would nowadays do the trick.


diff --git a/elf/dl-object.c b/elf/dl-object.c
index 0f594d2..de142de 100644
--- a/elf/dl-object.c
+++ b/elf/dl-object.c
@@ -62,6 +62,7 @@ _dl_new_object (char *realname, const char *libname, int type,
   size_t libname_len = strlen (libname) + 1;
   struct link_map *new;
   struct libname_list *newname;
+  static char rw_empty_string[] = { 0 };
 #ifdef SHARED
   /* We create the map for the executable before we know whether we have
      auditing libraries and if yes, how many.  Assume the worst.  */
@@ -88,7 +89,7 @@ _dl_new_object (char *realname, const char *libname, int type,
   /* newname->next = NULL;	We use calloc therefore not necessary.  */
   newname->dont_free = 1;
 
-  new->l_name = realname;
+  new->l_name = *realname ? realname : rw_empty_string;
   new->l_type = type;
   /* If we set the bit now since we know it is never used we avoid
      dirtying the cache line later.  */

Re: [PATCH] Skip VDSO when reading SO list

[Jan Kratochvil]

On Mon, 19 Aug 2013 20:29:07 +0200, Jan Kratochvil wrote:
> I believe the right fix is in glibc instead:
> 	[patch] Fix vDSO l_name for GDB's: Can't read pathname for load map:Input/output error.
> 	http://sourceware.org/ml/libc-alpha/2009-10/msg00001.html
> 	Message-ID: <20091004161706.GA27450@host0.dyn.jankratochvil.net>

I have re-posted the glibc fix, hopefully it gets resolved soon:
	[patchv2] Fix vDSO l_name for GDB's: Can't read pathname for load map: Input/output error.
	http://sourceware.org/ml/libc-alpha/2013-08/msg00364.html
	Message-ID: <20130820133807.GA15877@host2.jankratochvil.net>


Jan

Re: [PATCH] Skip VDSO when reading SO list

[Jan Kratochvil]

On Mon, 19 Aug 2013 16:44:13 +0200, Andreas Arnez wrote:
> 2013-08-19  Andreas Arnez  <arnez@linux.vnet.ibm.com>
> 
> 	* solib-svr4.c (svr4_read_so_list): Skip the VDSO when reading
                                                     vDSO
> 	link map entries.

Could you include here the testcase from:
	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-core-open-vdso-warning.patch


> Index: gdb/gdb/solib-svr4.c
> ===================================================================
> --- gdb.orig/gdb/solib-svr4.c
> +++ gdb/gdb/solib-svr4.c
> @@ -1310,6 +1310,7 @@ static int
>  svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
>  		   struct so_list ***link_ptr_ptr, int ignore_first)
>  {
> +  struct so_list *first = NULL;
>    CORE_ADDR next_lm;
>  
>    for (; lm != 0; prev_lm = lm, lm = next_lm)
> @@ -1349,10 +1350,22 @@ svr4_read_so_list (CORE_ADDR lm, CORE_AD
>  	{
>  	  struct svr4_info *info = get_svr4_info ();
>  
> +	  first = new;
>  	  info->main_lm_addr = new->lm_info->lm_addr;
>  	  do_cleanups (old_chain);
>  	  continue;
>  	}
> +
> +      /* The l_name of a VDSO sometimes lies in read-only memory that
                            vDSO
> +	 is excluded from a core dump.  In order to avoid the "can't
> +	 read pathname" warning, we try to identify the VDSO.  One
                                                        vDSO
> +	 criteria is that the l_name address matches that of the main
> +	 executable.  */
> +      if (first && new->lm_info->l_name == first->lm_info->l_name)

Here should be also '&& ignore_first'.


> +	{
> +	  do_cleanups (old_chain);
> +	  continue;
> +	}

And move this block below so that the condition is evaluated only if
target_read_string has really failed.

The purpose is that no workarounds should complicate the code in the case the
system components are already bug-free (after glibc gets fixed).


>  
>        /* Extract this shared object's name.  */
>        target_read_string (new->lm_info->l_name, &buffer,


Please make one new post after the updates.


Thanks,
Jan

Re: [PATCH] Skip VDSO when reading SO list

[Jan Kratochvil]

On Fri, 20 Sep 2013 15:15:49 +0200, Jan Kratochvil wrote:
> On Mon, 19 Aug 2013 16:44:13 +0200, Andreas Arnez wrote:
[...]
> > @@ -1349,10 +1350,22 @@ svr4_read_so_list (CORE_ADDR lm, CORE_AD
> >  	{
> >  	  struct svr4_info *info = get_svr4_info ();
> >  
> > +	  first = new;
> >  	  info->main_lm_addr = new->lm_info->lm_addr;
> >  	  do_cleanups (old_chain);
> >  	  continue;
> >  	}
> > +
> > +      /* The l_name of a VDSO sometimes lies in read-only memory that
>                             vDSO
> > +	 is excluded from a core dump.  In order to avoid the "can't
> > +	 read pathname" warning, we try to identify the VDSO.  One
>                                                         vDSO
> > +	 criteria is that the l_name address matches that of the main
> > +	 executable.  */
> > +      if (first && new->lm_info->l_name == first->lm_info->l_name)
> 
> Here should be also '&& ignore_first'.

I see now it is not needed.  FIRST would stay NULL in such case.

According to the GDB Coding standards s/first/first != NULL/ because FIRST is
a pointer.


Thanks,
Jan

Re: [PATCH] Skip VDSO when reading SO list

[Andreas Arnez]

Thanks for the comments!

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> On Mon, 19 Aug 2013 16:44:13 +0200, Andreas Arnez wrote:
>> 2013-08-19  Andreas Arnez  <arnez@linux.vnet.ibm.com>
>> 
>> 	* solib-svr4.c (svr4_read_so_list): Skip the VDSO when reading
>                                                      vDSO

OK, will replace all occurrences of 'VDSO' by 'vDSO'.

>> 	link map entries.
>
> Could you include here the testcase from:
> 	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-core-open-vdso-warning.patch

Sure.

>> Index: gdb/gdb/solib-svr4.c
>> ===================================================================
>> --- gdb.orig/gdb/solib-svr4.c
>> +++ gdb/gdb/solib-svr4.c
>> @@ -1310,6 +1310,7 @@ static int
>>  svr4_read_so_list (CORE_ADDR lm, CORE_ADDR prev_lm,
>>  		   struct so_list ***link_ptr_ptr, int ignore_first)
>>  {
>> +  struct so_list *first = NULL;
>>    CORE_ADDR next_lm;
>>  
>>    for (; lm != 0; prev_lm = lm, lm = next_lm)
>> @@ -1349,10 +1350,22 @@ svr4_read_so_list (CORE_ADDR lm, CORE_AD
>>  	{
>>  	  struct svr4_info *info = get_svr4_info ();
>>  
>> +	  first = new;
>>  	  info->main_lm_addr = new->lm_info->lm_addr;
>>  	  do_cleanups (old_chain);
>>  	  continue;
>>  	}
>> +
>> +      /* The l_name of a VDSO sometimes lies in read-only memory that
>                             vDSO
>> +	 is excluded from a core dump.  In order to avoid the "can't
>> +	 read pathname" warning, we try to identify the VDSO.  One
>                                                         vDSO
>> +	 criteria is that the l_name address matches that of the main
>> +	 executable.  */
>> +      if (first && new->lm_info->l_name == first->lm_info->l_name)
>
> Here should be also '&& ignore_first'.

Hm, this shouldn't be necessary, because 'first' is only set when
'ignore_first' is set.  Or did I miss something?

>> +	{
>> +	  do_cleanups (old_chain);
>> +	  continue;
>> +	}
>
> And move this block below so that the condition is evaluated only if
> target_read_string has really failed.
>
> The purpose is that no workarounds should complicate the code in the case the
> system components are already bug-free (after glibc gets fixed).

That's a good point.  Still, after thinking about this some more, I
prefer the order in the original patch, because it prevents a bogus
l_name from being detected in a second scan when the core dump is
debugged on a system with a different glibc version.  Users may also
experience this after a glibc update.  Thoughts?

Re: [PATCH] Skip VDSO when reading SO list

[Jan Kratochvil]

On Fri, 20 Sep 2013 19:00:07 +0200, Andreas Arnez wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> > Here should be also '&& ignore_first'.
> 
> Hm, this shouldn't be necessary, because 'first' is only set when
> 'ignore_first' is set.  Or did I miss something?

I agree now, replied in another mail.


> >> +	{
> >> +	  do_cleanups (old_chain);
> >> +	  continue;
> >> +	}
> >
> > And move this block below so that the condition is evaluated only if
> > target_read_string has really failed.
> >
> > The purpose is that no workarounds should complicate the code in the case the
> > system components are already bug-free (after glibc gets fixed).
> 
> That's a good point.  Still, after thinking about this some more, I
> prefer the order in the original patch, because it prevents a bogus
> l_name from being detected in a second scan when the core dump is
> debugged on a system with a different glibc version.  Users may also
> experience this after a glibc update.  Thoughts?

If you use non-matching glibc then it should complain.  In such case there is
a pointer to some real string so it is correct GDB tries to read it as
a library.  While the library load will fail it does not block trying to debug
such a core file.

A bit off-topic but for the non-matching libraries I should upstream the patch
(plus some related patches therein)
	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/gdb-6.6-buildid-locate.patch
as then it does not try to load non-matching executable/libraries at all.


Thanks,
Jan