[patch] Fix for PR gdb/10819

[patch] Fix for PR gdb/10819

[Paul Pluzhnikov]

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

Greetings,

Attached is a patch to fix PR gdb/10819
http://sourceware.org/bugzilla/show_bug.cgi?id=10819

Thanks,
-- 
Paul Pluzhnikov

2009-10-21  Paul Pluzhnikov  <ppluzhnikov@google.com>

        PR gdb/10819
        * dwarf2-frame.c (find_cie): Don't call bsearch on
        empty cie_table.

[-- Attachment #2: gdb-pr10819-20091021.txt --]
[-- Type: text/plain, Size: 439 bytes --]

--- dwarf2-frame.c.orig	2009-10-21 21:34:20.000000000 -0700
+++ dwarf2-frame.c	2009-10-21 21:35:25.000000000 -0700
@@ -1525,6 +1525,9 @@ find_cie (struct dwarf2_cie_table *cie_t
 {
   struct dwarf2_cie **p_cie;
 
+  if (cie_table->num_entries == 0)
+    return NULL;
+
   p_cie = bsearch (&cie_pointer, cie_table->entries, cie_table->num_entries,
                    sizeof (cie_table->entries[0]), bsearch_cie_cmp);
   if (p_cie != NULL)

Re: [patch] Fix for PR gdb/10819

[Jan Kratochvil]

On Thu, 22 Oct 2009 06:48:06 +0200, Paul Pluzhnikov wrote:
> Attached is a patch to fix PR gdb/10819
> http://sourceware.org/bugzilla/show_bug.cgi?id=10819
[...]
> +  if (cie_table->num_entries == 0)
> +    return NULL;
> +
>    p_cie = bsearch (&cie_pointer, cie_table->entries, cie_table->num_entries,
>                     sizeof (cie_table->entries[0]), bsearch_cie_cmp);
>    if (p_cie != NULL)

I would find appropriate there a comment it is a Solaris bug workaround, the
condition looks to me as clearly redundant there now.


Thanks,
Jan

Re: [patch] Fix for PR gdb/10819

[Paul Pluzhnikov]

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

On Wed, Oct 21, 2009 at 10:47 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:

> I would find appropriate there a comment it is a Solaris bug workaround, the
> condition looks to me as clearly redundant there now.

Comment added.

Should this patch (if approved) also go into gdb-7.0 branch?
(It is clearly very safe.)

Thanks,
-- 
Paul Pluzhnikov

2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

       PR gdb/10819
       * dwarf2-frame.c (find_cie): Don't call bsearch on
       empty cie_table.

[-- Attachment #2: gdb-pr10819-20091022.txt --]
[-- Type: text/plain, Size: 620 bytes --]

--- dwarf2-frame.c.orig	2009-10-21 21:34:20.000000000 -0700
+++ dwarf2-frame.c	2009-10-21 23:10:29.000000000 -0700
@@ -1525,6 +1525,14 @@ find_cie (struct dwarf2_cie_table *cie_t
 {
   struct dwarf2_cie **p_cie;
 
+  if (cie_table->num_entries == 0)
+    {
+      /* On Solaris 8 bsearch may call comparison function even when given
+	 an empty table.  As a work around, don't call bsearch under these
+	 conditions.  */
+      return NULL;
+    }
+
   p_cie = bsearch (&cie_pointer, cie_table->entries, cie_table->num_entries,
                    sizeof (cie_table->entries[0]), bsearch_cie_cmp);
   if (p_cie != NULL)

Re: [patch] Fix for PR gdb/10819

[Pedro Alves]

On Thursday 22 October 2009 07:14:21, Paul Pluzhnikov wrote:
> Comment added.

> +  if (cie_table->num_entries == 0)
> +    {
> +      /* On Solaris 8 bsearch may call comparison function even when given
> +        an empty table.  As a work around, don't call bsearch under these
> +        conditions.  */
> +      return NULL;
> +    }

>Apparently calling bsearch on a table with zero elements is unsafe on Solaris
>8.

FTR, so that this is archived, see:
 http://cvs.opensolaris.org/source/xref/pef/phase_I/usr/src/lib/libbc/libc/gen/common/bsearch.c

     43 	int two_width = width + width;
     44 	POINTER last = base + width * (nel - 1); /* Last element in table */
     45 
     46 	while (last >= base) {

The issue happens because you're passing a NULL BASE (your ENTRIES), so
LAST wraps around, and the while loop enters.  That bsearch assumes
BASE is a pointer into a valid object, which seems valid given
that BASE should point at an array of NEL objects.  You don't have
a table with zero elements, you have no table at all.  Note that
the solaris man page doesn't explicitly specify that when NEL is 0, the
compare function should not be called, no matter what.  opengroup.org
does, but that probably post dates the original bsearch appearences.

This seems to have been considered in more recent sources:
 http://cvs.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/util/bsearch.c

It is quite possible that other unix hosts have the same
valid-object assumption, if not by chance, because it's quite
possible that they've inherited the exact same bsearch.c
implementation.  I see that netbsd's implementation even
asserts (in devel builds only it seems) that base is not null.


There's another bsearch call in dwarf2-frame.c and another one
in objfiles.c (all recent and yours, it seems :-)).  Do they need
attention to the base==NULL or number-elements==0 case as well?

- 
Pedro Alves

Re: [patch] Fix for PR gdb/10819

[Andreas Schwab]

Pedro Alves <pedro@codesourcery.com> writes:

> On Thursday 22 October 2009 07:14:21, Paul Pluzhnikov wrote:
>> Comment added.
>
>> +  if (cie_table->num_entries == 0)
>> +    {
>> +      /* On Solaris 8 bsearch may call comparison function even when given
>> +        an empty table.  As a work around, don't call bsearch under these
>> +        conditions.  */
>> +      return NULL;
>> +    }
>
>>Apparently calling bsearch on a table with zero elements is unsafe on Solaris
>>8.
>
> FTR, so that this is archived, see:
>  http://cvs.opensolaris.org/source/xref/pef/phase_I/usr/src/lib/libbc/libc/gen/common/bsearch.c
>
>      43 	int two_width = width + width;
>      44 	POINTER last = base + width * (nel - 1); /* Last element in table */
>      45 
>      46 	while (last >= base) {
>
> The issue happens because you're passing a NULL BASE (your ENTRIES), so
> LAST wraps around, and the while loop enters.  That bsearch assumes
> BASE is a pointer into a valid object, which seems valid given
> that BASE should point at an array of NEL objects.

Note that this is what the C standard requires.  Even if the number of
elements is zero all pointer arguments must still be valid.

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [patch] Fix for PR gdb/10819

[Paul Pluzhnikov]

On Thu, Oct 22, 2009 at 4:09 AM, Andreas Schwab <schwab@redhat.com> wrote:

> Note that this is what the C standard requires.  Even if the number of
> elements is zero all pointer arguments must still be valid.

Specifically:

<quote>

In ISO/ IEC 9899:TC2 Committee Draft — May 6, 2005 WG14/N1124:

7.20.5 Searching and sorting utilities

1 These utilities make use of a comparison function to search or sort
arrays of unspecified type. Where an argument declared as size_t nmemb
specifies the length of the array for a function, nmemb can have the
value zero on a call to that function; the comparison function is not
called, a search finds no matching element, and sorting performs no
rearrangement. Pointer arguments on such a call shall still have valid
values, as described in 7.1.4.

And:

7.1.4 Use of library functions

1 Each of the following statements applies unless explicitly stated
otherwise in the detailed descriptions that follow: If an argument to
a function has an invalid value (such as a value outside the domain of
the function, or a pointer outside the address space of the program,
or a null pointer, or a pointer to non-modifiable storage when the
corresponding parameter is not const-qualified) or a type (after
promotion) not expected by a function with variable number of
arguments, the behavior is undefined. If a function argument is
described as being an array, the pointer actually passed to the
function shall have a value such that all address computations and
accesses to objects (that would be valid if the pointer did point to
the first element of such an array) are in fact valid.

</quote>

There appear to be quite a few qsort calls, and for a few I looked at
it's not immediately obvious that they pass a valid pointer when
nmemb==0 or that nmemb!=0.

I'll just fix the bsearch calls, since A) one of them is known to
cause a problem and B) I introduced them recently :-(

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for PR gdb/10819

[Paul Pluzhnikov]

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

On Thu, Oct 22, 2009 at 8:33 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> I'll just fix the bsearch calls, since A) one of them is known to
> cause a problem and B) I introduced them recently :-(

Here is the revised patch.

The call to bsearch in dwarf2_frame_find_fde is already preceded by
assert of "fde_table->num_entries > 0", so no changes are needed there.

There is one more call to bsearch in solib-osf.c, but that file appears
to not be used anymore. Ok to deleted it?

Thanks,
-- 
Paul Pluzhnikov

2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

      PR gdb/10819
      * dwarf2-frame.c (find_cie): Don't call bsearch on
      empty cie_table.
      * objfiles.c (find_pc_section): Likewise.

[-- Attachment #2: gdb-pr10819-20091022-2.txt --]
[-- Type: text/plain, Size: 1716 bytes --]

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.100
diff -u -p -u -r1.100 dwarf2-frame.c
--- dwarf2-frame.c	6 Oct 2009 23:27:04 -0000	1.100
+++ dwarf2-frame.c	22 Oct 2009 16:21:17 -0000
@@ -1525,6 +1525,22 @@ find_cie (struct dwarf2_cie_table *cie_t
 {
   struct dwarf2_cie **p_cie;
 
+  if (cie_table->num_entries == 0)
+    {
+      gdb_assert (cie_table->entries == NULL);
+
+      /* The C standard (ISO/IEC 9899:TC2) requires the BASE argument to
+	 bsearch be a valid pointer even when the NMEMB argument is 0.
+
+	 Passing NULL for BASE and 0 for NMEMB is also known to cause
+	 Solaris-8 bsearch to call bsearch_cie_cmp with NULL ELEMENT
+	 (which doesn't expect that and crashes); see PR gdb/10819.
+
+	 Therefore, avoid calling bsearch under these conditions.  */
+
+      return NULL;
+    }
+
   p_cie = bsearch (&cie_pointer, cie_table->entries, cie_table->num_entries,
                    sizeof (cie_table->entries[0]), bsearch_cie_cmp);
   if (p_cie != NULL)
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.99
diff -u -p -u -r1.99 objfiles.c
--- objfiles.c	19 Oct 2009 09:51:41 -0000	1.99
+++ objfiles.c	22 Oct 2009 16:21:17 -0000
@@ -1175,6 +1175,11 @@ find_pc_section (CORE_ADDR pc)
       pspace_info->objfiles_changed_p = 0;
     }
 
+  /* See comment in dwarf2-frame.c:find_cie on why this check
+     is necessary.  */
+  if (pspace_info->num_sections == 0)
+    return NULL;
+
   sp = (struct obj_section **) bsearch (&pc,
 					pspace_info->sections,
 					pspace_info->num_sections,

Re: [patch] Fix for PR gdb/10819

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> There is one more call to bsearch in solib-osf.c, but that file appears
Paul> to not be used anymore. Ok to deleted it?

It is referenced from config/alpha/alpha-osf3.mh, so I think it isn't
dead.

I would not worry about this call to bsearch.  It has been there since
revision 1.1 of that file, in 2001.  I think any problem it might
provoke probably would have been encountered by now.

Paul> +  if (cie_table->num_entries == 0)
Paul> +    {
Paul> +      gdb_assert (cie_table->entries == NULL);
Paul> +
Paul> +      /* The C standard (ISO/IEC 9899:TC2) requires the BASE argument to
Paul> +	 bsearch be a valid pointer even when the NMEMB argument is 0.
Paul> +
Paul> +	 Passing NULL for BASE and 0 for NMEMB is also known to cause
Paul> +	 Solaris-8 bsearch to call bsearch_cie_cmp with NULL ELEMENT
Paul> +	 (which doesn't expect that and crashes); see PR gdb/10819.
Paul> +
Paul> +	 Therefore, avoid calling bsearch under these conditions.  */

I'm ok with this paragraph but given that this is a C standard thing, it
could really just say something like "The C89 Standard requires BASE to
be non-NULL".

Paul> +  /* See comment in dwarf2-frame.c:find_cie on why this check
Paul> +     is necessary.  */

I'm not ok with this comment; references like this are fragile because
the referenced comment may change without anybody knowing to update this
one.

This patch is ok if you replace the second comment with something
self-contained.

thanks,
Tom

Re: [patch] Fix for PR gdb/10819

[Paul Pluzhnikov]

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

On Thu, Oct 22, 2009 at 10:43 AM, Tom Tromey <tromey@redhat.com> wrote:

> Paul> +  /* See comment in dwarf2-frame.c:find_cie on why this check
> Paul> +     is necessary.  */
>
> I'm not ok with this comment; references like this are fragile because
> the referenced comment may change without anybody knowing to update this
> one.

How about this one then?
Tested on Linux/x86_64, no regressions.

Thanks,
-- 
Paul Pluzhnikov

2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

     PR gdb/10819
     * dwarf2-frame.c (find_cie): Don't call bsearch on
     empty cie_table.
     * objfiles.c (find_pc_section): Likewise.
     (update_section_map): Don't allocate empty table.

[-- Attachment #2: gdb-pr10819-20091022-3.txt --]
[-- Type: text/plain, Size: 1877 bytes --]

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.100
diff -u -p -u -r1.100 dwarf2-frame.c
--- dwarf2-frame.c	6 Oct 2009 23:27:04 -0000	1.100
+++ dwarf2-frame.c	22 Oct 2009 18:26:31 -0000
@@ -1525,6 +1525,14 @@ find_cie (struct dwarf2_cie_table *cie_t
 {
   struct dwarf2_cie **p_cie;
 
+  /* The C standard (ISO/IEC 9899:TC2) requires the BASE argument to
+     bsearch be non-NULL.  */
+  if (cie_table->entries == NULL)
+    {
+      gdb_assert (cie_table->num_entries == 0);
+      return NULL;
+    }
+
   p_cie = bsearch (&cie_pointer, cie_table->entries, cie_table->num_entries,
                    sizeof (cie_table->entries[0]), bsearch_cie_cmp);
   if (p_cie != NULL)
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.99
diff -u -p -u -r1.99 objfiles.c
--- objfiles.c	19 Oct 2009 09:51:41 -0000	1.99
+++ objfiles.c	22 Oct 2009 18:26:31 -0000
@@ -1113,6 +1113,14 @@ update_section_map (struct program_space
       if (insert_section_p (objfile->obfd, s->the_bfd_section))
 	alloc_size += 1;
 
+  /* This happens on detach/attach (e.g. in gdb.base/attach.exp).  */
+  if (alloc_size == 0)
+    {
+      *pmap = NULL;
+      *pmap_size = 0;
+      return;
+    }
+
   map = xmalloc (alloc_size * sizeof (*map));
 
   i = 0;
@@ -1175,6 +1183,14 @@ find_pc_section (CORE_ADDR pc)
       pspace_info->objfiles_changed_p = 0;
     }
 
+  /* The C standard (ISO/IEC 9899:TC2) requires the BASE argument to
+     bsearch be non-NULL.  */
+  if (pspace_info->sections == NULL)
+    {
+      gdb_assert (pspace_info->num_sections == 0);
+      return NULL;
+    }
+
   sp = (struct obj_section **) bsearch (&pc,
 					pspace_info->sections,
 					pspace_info->num_sections,

Re: [patch] Fix for PR gdb/10819

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> How about this one then?
Paul> Tested on Linux/x86_64, no regressions.

Great, thanks.

Tom

Re: [patch] Fix for PR gdb/10819

[Paul Pluzhnikov]

On Thu, Oct 22, 2009 at 1:13 PM, Tom Tromey <tromey@redhat.com> wrote:

> Paul> How about this one then?

> Great, thanks.

Committed on trunk and (with minor adjustment) on gdb_7_0-branch.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for PR gdb/10819

[Pedro Alves]

On Thursday 22 October 2009 07:14:21, Paul Pluzhnikov wrote:
> Should this patch (if approved) also go into gdb-7.0 branch?
> (It is clearly very safe.)

I think that's a good idea.

-- 
Pedro Alves