[PATCH,gdb]: ensures that cie ptr of an fda is a cie

[PATCH,gdb]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

Here is a patch that ensures that cie pointer of a fda really points to a cie.

This avoids infinite loops if a fda cie pointer points to itself, as was the case on
mac when the mmaped read section was broken for fat binaries and returned 
invalid info (see http://sourceware.org/bugzilla/show_bug.cgi?id=11488 ).

This increases the robustness of gdb, so I think it is worthwhile to add.

Fawzi

2011-07-04 Fawzi Mohamed <fawzi.mohamed@nokia.com>

	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): ensure
	that cie pointer really points to a cie 

Index: gdb/dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.125
diff -d -u -r1.125 dwarf2-frame.c
--- gdb/dwarf2-frame.c	4 Jul 2011 16:30:09 -0000	1.125
+++ gdb/dwarf2-frame.c	4 Jul 2011 17:39:29 -0000
@@ -1801,17 +1801,25 @@
 #define DW64_CIE_ID ~0
 #endif
 
+enum eh_frame_type {
+    eh_cie_type_id = 1,
+    eh_fde_type_id = 2,
+    eh_cie_or_fde_type_id = 3
+};
+
 static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
 				     int eh_frame_p,
                                      struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
 
 /* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
    the next byte to be processed.  */
 static gdb_byte *
 decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                       struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   gdb_byte *buf, *end;
@@ -1862,6 +1870,8 @@
       char *augmentation;
       unsigned int cie_version;
 
+      gdb_assert ((entry_type & eh_cie_type_id)!=0);
+
       /* Record the offset into the .debug_frame section of this CIE.  */
       cie_pointer = start - unit->dwarf_frame_buffer;
 
@@ -2027,6 +2037,8 @@
       /* This is a FDE.  */
       struct dwarf2_fde *fde;
 
+      gdb_assert ((entry_type & eh_fde_type_id)!=0);
+
       /* In an .eh_frame section, the CIE pointer is the delta between the
 	 address within the FDE where the CIE pointer is stored and the
 	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2060,8 @@
       if (fde->cie == NULL)
 	{
 	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      eh_cie_type_id);
 	  fde->cie = find_cie (cie_table, cie_pointer);
 	}
 
@@ -2093,7 +2106,8 @@
 static gdb_byte *
 decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                     struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
 {
   enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
   gdb_byte *ret;
@@ -2102,7 +2116,8 @@
   while (1)
     {
       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+                                  cie_table, fde_table,entry_type,
+                                  entry_type);
       if (ret != NULL)
 	break;
 
@@ -2256,7 +2271,8 @@
           frame_ptr = unit->dwarf_frame_buffer;
           while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
             frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+                                            &cie_table, &fde_table,
+                                            eh_cie_or_fde_type_id);
 
           if (cie_table.num_entries != 0)
             {

Re: [PATCH,gdb]: ensures that cie ptr of an fda is a cie

[Joel Brobecker]

> This increases the robustness of gdb, so I think it is worthwhile to add.

Indeed.

> 2011-07-04 Fawzi Mohamed <fawzi.mohamed@nokia.com>
> 
> 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): ensure
> 	that cie pointer really points to a cie 

I'm not a specialist of this area of the code, so I can't approve.
However, I'm noticing some style issues (style is project-specific).
Also, please make sure to use the '-p' switch when creating the diff,
so that we can get a little more context for each hunk (see man diff).

> +enum eh_frame_type {
> +    eh_cie_type_id = 1,
> +    eh_fde_type_id = 2,
> +    eh_cie_or_fde_type_id = 3
> +};

The open curly brace should be on the next line, and our indentation
level is 2 characters, thus:

    enum eh_frame_type
    {
      eh_cie_type_id = 1,
      eh_fde_type_id = 2,
      eh_cie_or_fde_type_id = 3
    };

Also, we try to document every type and every function that we add
in GDB. And for types like this, it is often advantageous to also
document each element (not sure in this case). You can have a look
at struct frame_id in frame.h for an example of how we document
our types.

>  static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
>  				     int eh_frame_p,
>                                       struct dwarf2_cie_table *cie_table,
> -                                     struct dwarf2_fde_table *fde_table);
> +                                     struct dwarf2_fde_table *fde_table,
> +                                     enum eh_frame_type entry_type);

The documentation of these function need to be augmented in order
to describe what this extra parameter does.  Same for all the other
functions that you have modified.

> +      gdb_assert ((entry_type & eh_cie_type_id)!=0);

Style: spaces around the != operator.  Also, please provide a comment
explaining why entry_type & eh_cie_type_id can never be zero.  It might
be obvious if already specified in the function documention, but might
not be so obvious. In fact, what is the purpose of this assertion?
It looks like you're validating the value of entry_type to make sure
that it is either a eh_cie_type_id or a eh_cie_or_fde_type_id.
This leads me to believe that the values inside eh_frame_type
are meant to be bit flags. This would be much clearer with documentation
and by using the following syntax:

    enum eh_frame_type
    {
      eh_cie_type_id = 1 << 0,
      eh_fde_type_id = 1 << 1,
      eh_cie_or_fde_type_id = eh_cie_type_id + eh_fde_type_id
    };

(I might also add an `unknown' value that's equal to zero, just
to name that value).

> +      gdb_assert ((entry_type & eh_fde_type_id)!=0);
> +

Same comment.

-- 
Joel

Re: [PATCH,gdb, Update1]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

On Jul 4, 2011, at 8:41 PM, ext Joel Brobecker wrote:

>> This increases the robustness of gdb, so I think it is worthwhile to add.
> 
> Indeed.
> [...]

Thanks for the comments Joel, here is a revised patch that hopefully addresses your issues

Fawzi

2011-07-04 Fawzi Mohamed <fawzi.mohamed@nokia.com>

	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): ensure
	that CIE pointer of an FDE really points to a CIE 

Index: gdb/dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.125
diff -p -d -u -r1.125 dwarf2-frame.c
--- gdb/dwarf2-frame.c	4 Jul 2011 16:30:09 -0000	1.125
+++ gdb/dwarf2-frame.c	5 Jul 2011 09:07:59 -0000
@@ -1801,17 +1801,28 @@ add_fde (struct dwarf2_fde_table *fde_ta
 #define DW64_CIE_ID ~0
 #endif
 
+/* defines the type of eh_frames that are expected to be decoded: CIE, FDE
+   or any of them */
+enum eh_frame_type
+{
+  eh_cie_type_id = 1<<0,
+  eh_fde_type_id = 1<<1,
+  eh_cie_or_fde_type_id = eh_cie_type_id + eh_fde_type_id
+};
+
 static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
 				     int eh_frame_p,
                                      struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
 
-/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
-   the next byte to be processed.  */
+/* Decode the next CIE or FDE, entry type specifies the expected type.
+   Return NULL if invalid input, otherwise the next byte to be processed.  */
 static gdb_byte *
 decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                       struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   gdb_byte *buf, *end;
@@ -1862,6 +1873,9 @@ decode_frame_entry_1 (struct comp_unit *
       char *augmentation;
       unsigned int cie_version;
 
+      /* checks that we expected a CIE */
+      gdb_assert ((entry_type & eh_cie_type_id) != 0);
+
       /* Record the offset into the .debug_frame section of this CIE.  */
       cie_pointer = start - unit->dwarf_frame_buffer;
 
@@ -2027,6 +2041,9 @@ decode_frame_entry_1 (struct comp_unit *
       /* This is a FDE.  */
       struct dwarf2_fde *fde;
 
+      /* checks that we expected a FDE */
+      gdb_assert ((entry_type & eh_fde_type_id) != 0);
+
       /* In an .eh_frame section, the CIE pointer is the delta between the
 	 address within the FDE where the CIE pointer is stored and the
 	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2065,8 @@ decode_frame_entry_1 (struct comp_unit *
       if (fde->cie == NULL)
 	{
 	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      eh_cie_type_id);
 	  fde->cie = find_cie (cie_table, cie_pointer);
 	}
 
@@ -2089,11 +2107,12 @@ decode_frame_entry_1 (struct comp_unit *
   return end;
 }
 
-/* Read a CIE or FDE in BUF and decode it.  */
+/* Read a CIE or FDE in BUF and decode it. entry type specifies if we expect an FDE or a CIE */
 static gdb_byte *
 decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                     struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
 {
   enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
   gdb_byte *ret;
@@ -2102,7 +2121,8 @@ decode_frame_entry (struct comp_unit *un
   while (1)
     {
       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+                                  cie_table, fde_table,entry_type,
+                                  entry_type);
       if (ret != NULL)
 	break;
 
@@ -2256,7 +2276,8 @@ dwarf2_build_frame_info (struct objfile 
           frame_ptr = unit->dwarf_frame_buffer;
           while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
             frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+                                            &cie_table, &fde_table,
+                                            eh_cie_or_fde_type_id);
 
           if (cie_table.num_entries != 0)
             {

Re: [PATCH,gdb, Update1]: ensures that cie ptr of an fda is a cie

[Joel Brobecker]

> 2011-07-04 Fawzi Mohamed <fawzi.mohamed@nokia.com>
> 
> 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): ensure
> 	that CIE pointer of an FDE really points to a CIE 

Just a few more nits... (a lot of rules, I know, but we try to have
a consistent development style).

First, in the ChangeLog above: 2 spaces between the date and your
name, and before your email address.  Also, sentences always start
with a capital letter, and end with a period.  Thus

2011-07-04  Fawzi Mohamed  <fawzi.mohamed@nokia.com>

  	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
  	that CIE pointer of an FDE really points to a CIE .

> +/* defines the type of eh_frames that are expected to be decoded: CIE, FDE
> +   or any of them */
> +enum eh_frame_type

Empty line between documentation comment and start of definition...

> +{
> +  eh_cie_type_id = 1<<0,
> +  eh_fde_type_id = 1<<1,
> +  eh_cie_or_fde_type_id = eh_cie_type_id + eh_fde_type_id
> +};

Spaces around binary operators => `1 << 0'.

> -/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
> -   the next byte to be processed.  */
> +/* Decode the next CIE or FDE, entry type specifies the expected type.
> +   Return NULL if invalid input, otherwise the next byte to be processed.  */
>  static gdb_byte *

Same here...

> +      /* checks that we expected a CIE */
> +      gdb_assert ((entry_type & eh_cie_type_id) != 0);

Same here as well. But I think you meant something like `We expect
a CIE.'.


-- 
Joel

Re: [PATCH,gdb, Update2]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

On Jul 5, 2011, at 5:14 PM, ext Joel Brobecker wrote:

>> 2011-07-04 Fawzi Mohamed <fawzi.mohamed@nokia.com>
>> 
>> 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): ensure
>> 	that CIE pointer of an FDE really points to a CIE 
> 
> Just a few more nits... (a lot of rules, I know, but we try to have
> a consistent development style).

eh eh I sort of expected it, so I put a number after update in the subject :)

> [...]
> Empty line between documentation comment and start of definition...

this doesn't seem to be much followed in this file... I am fixing it for the functions I touch...
but I suppose it doesn't apply for in function comments, as it is never used like that in this file.

> [...]
>> +      /* checks that we expected a CIE */
>> +      gdb_assert ((entry_type & eh_cie_type_id) != 0);
> 
> Same here as well. But I think you meant something like `We expect
> a CIE.'.

actually no at that point he found a CIE but he has to check that a CIE was actually expected.
I have reworded it to /* Check that a CIE was expected. */

another thing that I have noted os 0x0c characters in the file (new page in ascii), not sure why they are there.

anyway here is the updated patch

2011-07-04  Fawzi Mohamed  <fawzi.mohamed@nokia.com>

 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
 	that CIE pointer of an FDE really points to a CIE .

Index: gdb/dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.125
diff -f -d -u -r1.125 dwarf2-frame.c
--- gdb/dwarf2-frame.c	4 Jul 2011 16:30:09 -0000	1.125
+++ gdb/dwarf2-frame.c	5 Jul 2011 16:17:57 -0000
@@ -1801,17 +1801,30 @@
 #define DW64_CIE_ID ~0
 #endif
 
+/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
+   or any of them. */
+
+enum eh_frame_type
+{
+  eh_cie_type_id = 1 << 0,
+  eh_fde_type_id = 1 << 1,
+  eh_cie_or_fde_type_id = eh_cie_type_id + eh_fde_type_id
+};
+
 static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
 				     int eh_frame_p,
                                      struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
+
+/* Decode the next CIE or FDE, entry_type specifies the expected type.
+   Return NULL if invalid input, otherwise the next byte to be processed.  */
 
-/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
-   the next byte to be processed.  */
 static gdb_byte *
 decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                       struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   gdb_byte *buf, *end;
@@ -1862,6 +1875,9 @@
       char *augmentation;
       unsigned int cie_version;
 
+      /* Check that a CIE was expected. */
+      gdb_assert ((entry_type & eh_cie_type_id) != 0);
+
       /* Record the offset into the .debug_frame section of this CIE.  */
       cie_pointer = start - unit->dwarf_frame_buffer;
 
@@ -2027,6 +2043,9 @@
       /* This is a FDE.  */
       struct dwarf2_fde *fde;
 
+      /* Check that an FDE was expected. */
+      gdb_assert ((entry_type & eh_fde_type_id) != 0);
+
       /* In an .eh_frame section, the CIE pointer is the delta between the
 	 address within the FDE where the CIE pointer is stored and the
 	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2067,8 @@
       if (fde->cie == NULL)
 	{
 	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      eh_cie_type_id);
 	  fde->cie = find_cie (cie_table, cie_pointer);
 	}
 
@@ -2089,11 +2109,14 @@
   return end;
 }
 
-/* Read a CIE or FDE in BUF and decode it.  */
+/* Read a CIE or FDE in BUF and decode it. Entry_type specifies if we expect
+   an FDE or a CIE. */
+
 static gdb_byte *
 decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                     struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
 {
   enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
   gdb_byte *ret;
@@ -2102,7 +2125,7 @@
   while (1)
     {
       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+                                  cie_table, fde_table,entry_type);
       if (ret != NULL)
 	break;
 
@@ -2256,7 +2279,8 @@
           frame_ptr = unit->dwarf_frame_buffer;
           while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
             frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+                                            &cie_table, &fde_table,
+                                            eh_cie_or_fde_type_id);
 
           if (cie_table.num_entries != 0)
             {
@@ -2277,7 +2301,8 @@
       frame_ptr = unit->dwarf_frame_buffer;
       while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
 	frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
-                                        &cie_table, &fde_table);
+                                        &cie_table, &fde_table,
+                                        eh_cie_or_fde_type_id);
     }
 
   /* Discard the cie_table, it is no longer needed.  */

Re: [PATCH,gdb]: ensures that cie ptr of an fda is a cie

[Tom Tromey]

>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:

Fawzi> +enum eh_frame_type {
Fawzi> +    eh_cie_type_id = 1,
Fawzi> +    eh_fde_type_id = 2,
Fawzi> +    eh_cie_or_fde_type_id = 3

I think enum constant names should be all caps.
That is the usual style.

Fawzi> +      gdb_assert ((entry_type & eh_cie_type_id)!=0);

It seems to me that this code is attempting to validate user input.  If
so, an assertion is incorrect -- instead it should call either error or
warning and in the latter case also arrange for the bad data to be
ignored.

Tom

Re: [PATCH,gdb,Update3]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

On Jul 5, 2011, at 6:25 PM, ext Tom Tromey wrote:

>>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:
> [...]
> I think enum constant names should be all caps.
> That is the usual style.

Ok thanks.

> Fawzi> +      gdb_assert ((entry_type & eh_cie_type_id)!=0);
> 
> It seems to me that this code is attempting to validate user input.  If
> so, an assertion is incorrect -- instead it should call either error or
> warning and in the latter case also arrange for the bad data to be
> ignored.

What I do is to detect corrupt eh_sections. I have to say that I didn't see eh_section as user input, but for a debugger it makes sense.
I have used an error, because (at least in my case) this was triggered by another gdb bug, so I guess that it is seldom enough that recovery is not required (for recovery I guess one should nuke cie_table and fde_table, and then artificially advance or try to reach the error handler, but I am not 100% sure).

The error message has a dot at the end there doesn't seem to be a consensus on that, but I hope it is ok.

So here is the updated patch:

2011-07-04  Fawzi Mohamed  <fawzi.mohamed@nokia.com>

	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
	that CIE pointer of an FDE really points to a CIE .
Index: gdb/dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.125
diff -f -d -u -r1.125 dwarf2-frame.c
--- gdb/dwarf2-frame.c	4 Jul 2011 16:30:09 -0000	1.125
+++ gdb/dwarf2-frame.c	6 Jul 2011 10:04:52 -0000
@@ -1801,17 +1801,30 @@
#define DW64_CIE_ID ~0
#endif

+/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
+   or any of them. */
+
+enum eh_frame_type
+{
+  EH_CIE_TYPE_ID = 1 << 0,
+  EH_FDE_TYPE_ID = 1 << 1,
+  EH_CIE_OR_FDE_TYPE_ID = EH_CIE_TYPE_ID + EH_FDE_TYPE_ID
+};
+
static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
				     int eh_frame_p,
                                     struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
+
+/* Decode the next CIE or FDE, entry_type specifies the expected type.
+   Return NULL if invalid input, otherwise the next byte to be processed.  */

-/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
-   the next byte to be processed.  */
static gdb_byte *
decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                      struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
{
  struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
  gdb_byte *buf, *end;
@@ -1862,6 +1875,10 @@
      char *augmentation;
      unsigned int cie_version;

+      /* Check that a CIE was expected. */
+      if ((entry_type & EH_CIE_TYPE_ID) == 0)
+	error (_("Found a CIE when not expecting it, broken eh_info"));
+
      /* Record the offset into the .debug_frame section of this CIE.  */
      cie_pointer = start - unit->dwarf_frame_buffer;

@@ -2027,6 +2044,10 @@
      /* This is a FDE.  */
      struct dwarf2_fde *fde;

+      /* Check that an FDE was expected. */
+      if ((entry_type & EH_FDE_TYPE_ID) == 0)
+	error (_("Found an FDE when not expecting it, broken eh_info."));
+
      /* In an .eh_frame section, the CIE pointer is the delta between the
	 address within the FDE where the CIE pointer is stored and the
	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2069,8 @@
      if (fde->cie == NULL)
	{
	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      EH_CIE_TYPE_ID);
	  fde->cie = find_cie (cie_table, cie_pointer);
	}

@@ -2089,11 +2111,14 @@
  return end;
}

-/* Read a CIE or FDE in BUF and decode it.  */
+/* Read a CIE or FDE in BUF and decode it. Entry_type specifies if we expect
+   an FDE or a CIE. */
+
static gdb_byte *
decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                    struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
{
  enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
  gdb_byte *ret;
@@ -2102,7 +2127,7 @@
  while (1)
    {
      ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+				  cie_table, fde_table,entry_type);
      if (ret != NULL)
	break;

@@ -2256,7 +2281,8 @@
          frame_ptr = unit->dwarf_frame_buffer;
          while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
            frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+                                            &cie_table, &fde_table,
+                                            EH_CIE_OR_FDE_TYPE_ID);

          if (cie_table.num_entries != 0)
            {
@@ -2277,7 +2303,8 @@
      frame_ptr = unit->dwarf_frame_buffer;
      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
	frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
-                                        &cie_table, &fde_table);
+                                        &cie_table, &fde_table,
+                                        EH_CIE_OR_FDE_TYPE_ID);
    }

  /* Discard the cie_table, it is no longer needed.  */

Re: [PATCH,gdb,Update3]: ensures that cie ptr of an fda is a cie

[Jan Kratochvil]

On Wed, 06 Jul 2011 16:40:14 +0200, Fawzi Mohamed wrote:
> +/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
> +   or any of them. */
                    ^^^ Two spaces at the end of line.

> +
> +enum eh_frame_type
> +{
> +  EH_CIE_TYPE_ID = 1 << 0,
> +  EH_FDE_TYPE_ID = 1 << 1,
> +  EH_CIE_OR_FDE_TYPE_ID = EH_CIE_TYPE_ID + EH_FDE_TYPE_ID

A nitpick but the intended operation is | , not + .


[...]
> @@ -2102,7 +2127,7 @@
>   while (1)
>     {
>       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
> -                                  cie_table, fde_table);
> +				  cie_table, fde_table,entry_type);
   				                      ^^ missing space (' ')


Thanks for the fix,
Jan

Re: [PATCH,gdb,Update4]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

On Jul 6, 2011, at 7:03 PM, ext Jan Kratochvil wrote:

> On Wed, 06 Jul 2011 16:40:14 +0200, Fawzi Mohamed wrote:
>> +/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
>> +   or any of them. */
>                    ^^^ Two spaces at the end of line.

done

>> +
>> +enum eh_frame_type
>> +{
>> +  EH_CIE_TYPE_ID = 1 << 0,
>> +  EH_FDE_TYPE_ID = 1 << 1,
>> +  EH_CIE_OR_FDE_TYPE_ID = EH_CIE_TYPE_ID + EH_FDE_TYPE_ID
> 
> A nitpick but the intended operation is | , not + .

well for non overlapping masks + is the same, and as it was what had been proposed I used it, but I also find | better.

In the previous version I had switched to error from gdb_assert, but had not yet controlled to which handler it went.
The handler is in solib_read_symbols, and it is silent if one uses it inside an IDE (no tty). That skips the whole so lib, so I have added handlers to dwarf2_build_frame_info that prints a warning and skip only the corrupted eh_fame section.

Here is updated patch:

2011-07-07  Fawzi Mohamed  <fawzi.mohamed@nokia.com>

	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
	that CIE pointer of an FDE really points to a CIE .

diff --git a/fsfGdb.config b/fsfGdb.config
index e69de29..8cec188 100644
--- a/fsfGdb.config
+++ b/fsfGdb.config
@@ -0,0 +1 @@
+// ADD PREDEFINED MACROS HERE!
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index b68a773..a3c19ee 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1801,17 +1801,30 @@ add_fde (struct dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
 #define DW64_CIE_ID ~0
 #endif
 
+/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
+   or any of them.  */
+
+enum eh_frame_type
+{
+  EH_CIE_TYPE_ID = 1 << 0,
+  EH_FDE_TYPE_ID = 1 << 1,
+  EH_CIE_OR_FDE_TYPE_ID = EH_CIE_TYPE_ID | EH_FDE_TYPE_ID
+};
+
 static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
 				     int eh_frame_p,
                                      struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
+
+/* Decode the next CIE or FDE, entry_type specifies the expected type.
+   Return NULL if invalid input, otherwise the next byte to be processed.  */
 
-/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
-   the next byte to be processed.  */
 static gdb_byte *
 decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                       struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   gdb_byte *buf, *end;
@@ -1862,6 +1875,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       char *augmentation;
       unsigned int cie_version;
 
+      /* Check that a CIE was expected.  */
+      if ((entry_type & EH_CIE_TYPE_ID) == 0)
+	error (_("Found a CIE when not expecting it, broken eh_info"));
+
       /* Record the offset into the .debug_frame section of this CIE.  */
       cie_pointer = start - unit->dwarf_frame_buffer;
 
@@ -2027,6 +2044,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       /* This is a FDE.  */
       struct dwarf2_fde *fde;
 
+      /* Check that an FDE was expected.  */
+      if ((entry_type & EH_FDE_TYPE_ID) == 0)
+	error (_("Found an FDE when not expecting it, broken eh_info."));
+
       /* In an .eh_frame section, the CIE pointer is the delta between the
 	 address within the FDE where the CIE pointer is stored and the
 	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2069,8 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       if (fde->cie == NULL)
 	{
 	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      EH_CIE_TYPE_ID);
 	  fde->cie = find_cie (cie_table, cie_pointer);
 	}
 
@@ -2089,11 +2111,14 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   return end;
 }
 
-/* Read a CIE or FDE in BUF and decode it.  */
+/* Read a CIE or FDE in BUF and decode it. Entry_type specifies if we expect
+   an FDE or a CIE.  */
+
 static gdb_byte *
 decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                     struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
 {
   enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
   gdb_byte *ret;
@@ -2102,7 +2127,7 @@ decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   while (1)
     {
       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+				  cie_table, fde_table, entry_type);
       if (ret != NULL)
 	break;
 
@@ -2212,6 +2237,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
   struct dwarf2_cie_table cie_table;
   struct dwarf2_fde_table fde_table;
   struct dwarf2_fde_table *fde_table2;
+  volatile struct gdb_exception e;
 
   cie_table.num_entries = 0;
   cie_table.entries = NULL;
@@ -2253,10 +2279,27 @@ dwarf2_build_frame_info (struct objfile *objfile)
           if (txt)
             unit->tbase = txt->vma;
 
-          frame_ptr = unit->dwarf_frame_buffer;
-          while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-            frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+	  TRY_CATCH (e, RETURN_MASK_ERROR)
+	    {
+	      frame_ptr = unit->dwarf_frame_buffer;
+	      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+		frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
+						&cie_table, &fde_table,
+						EH_CIE_OR_FDE_TYPE_ID);
+	    }
+
+	  if (e.reason < 0)
+	    {
+	      warning (_("skipping .eh_frame info of %s: %s"),objfile->name, e.message);
+
+	      if (fde_table.num_entries != 0)
+		{
+		  xfree(fde_table.entries);
+		  fde_table.entries = NULL;
+		  fde_table.num_entries = 0;
+		}
+	      /* cie_table is discarded by the next if.  */
+	    }
 
           if (cie_table.num_entries != 0)
             {
@@ -2274,10 +2317,37 @@ dwarf2_build_frame_info (struct objfile *objfile)
                            &unit->dwarf_frame_size);
   if (unit->dwarf_frame_size)
     {
-      frame_ptr = unit->dwarf_frame_buffer;
-      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-	frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
-                                        &cie_table, &fde_table);
+      int num_old_fde_entries = fde_table.num_entries;
+
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  frame_ptr = unit->dwarf_frame_buffer;
+	  while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+	    frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
+					    &cie_table, &fde_table,
+					    EH_CIE_OR_FDE_TYPE_ID);
+	}
+      if (e.reason < 0)
+	{
+	  warning (_("skipping .debug_frame info of %s: %s"),objfile->name, e.message);
+
+	  if (fde_table.num_entries != 0)
+	    {
+	      fde_table.num_entries = num_old_fde_entries;
+	      if (num_old_fde_entries == 0)
+		{
+		  xfree(fde_table.entries);
+		  fde_table.entries = NULL;
+		}
+	      else
+		{
+		    xrealloc (fde_table.entries,
+			      fde_table.num_entries * sizeof (fde_table.entries[0]));
+		}
+	    }
+	  fde_table.num_entries = num_old_fde_entries;
+	  /* cie_table is discarded by the next if  */
+	}
     }
 
   /* Discard the cie_table, it is no longer needed.  */

Re: [PATCH,gdb,Update4]: ensures that cie ptr of an fda is a cie

[Tom Tromey]

>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:

Fawzi> 2011-07-07  Fawzi Mohamed  <fawzi.mohamed@nokia.com>
Fawzi> 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
Fawzi> 	that CIE pointer of an FDE really points to a CIE .

Looks pretty good.

Fawzi> +	error (_("Found a CIE when not expecting it, broken eh_info"));

Remove ", broken eh_info" here.

Fawzi> +	error (_("Found an FDE when not expecting it, broken eh_info."));

And here.

The reason is that the error that is printed mentions .eh_frame, and I
think that is sufficient.

Fawzi> +	      warning (_("skipping .eh_frame info of %s: %s"),objfile->name, e.message);

Line too long.  Also, missing a space after the first "," -- but that is
where you should split the line anyhow.

Fawzi> +		  xfree(fde_table.entries);

Space before the paren.

Fawzi> +	  warning (_("skipping .debug_frame info of %s: %s"),objfile->name, e.message);

Likewise.

Fawzi> +		  xfree(fde_table.entries);

Space.

Fawzi> +		{
Fawzi> +		    xrealloc (fde_table.entries,
Fawzi> +			      fde_table.num_entries * sizeof (fde_table.entries[0]));

Wrong indentation for the xrealloc.

I think it is weird not to use the result of the xrealloc call, even
though we know it is just shrinking the buffer.

Tom

Re: [PATCH,gdb,Update6]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

On Jul 7, 2011, at 9:37 PM, ext Tom Tromey wrote:

> [...]

Tom, thanks for the comments, I did the changes.

> I think it is weird not to use the result of the xrealloc call, even
> though we know it is just shrinking the buffer.

I added a comment about that.

Fawzi

2011-07-07  Fawzi Mohamed  <fawzi.mohamed@nokia.com>

	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
	that CIE pointer of an FDE really points to a CIE .

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index b68a773..18d74b4 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1801,17 +1801,30 @@ add_fde (struct dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
 #define DW64_CIE_ID ~0
 #endif
 
+/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
+   or any of them.  */
+
+enum eh_frame_type
+{
+  EH_CIE_TYPE_ID = 1 << 0,
+  EH_FDE_TYPE_ID = 1 << 1,
+  EH_CIE_OR_FDE_TYPE_ID = EH_CIE_TYPE_ID | EH_FDE_TYPE_ID
+};
+
 static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
 				     int eh_frame_p,
                                      struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
+
+/* Decode the next CIE or FDE, entry_type specifies the expected type.
+   Return NULL if invalid input, otherwise the next byte to be processed.  */
 
-/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
-   the next byte to be processed.  */
 static gdb_byte *
 decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                       struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   gdb_byte *buf, *end;
@@ -1862,6 +1875,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       char *augmentation;
       unsigned int cie_version;
 
+      /* Check that a CIE was expected.  */
+      if ((entry_type & EH_CIE_TYPE_ID) == 0)
+	error (_("Found a CIE when not expecting it."));
+
       /* Record the offset into the .debug_frame section of this CIE.  */
       cie_pointer = start - unit->dwarf_frame_buffer;
 
@@ -2027,6 +2044,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       /* This is a FDE.  */
       struct dwarf2_fde *fde;
 
+      /* Check that an FDE was expected.  */
+      if ((entry_type & EH_FDE_TYPE_ID) == 0)
+	error (_("Found an FDE when not expecting it."));
+
       /* In an .eh_frame section, the CIE pointer is the delta between the
 	 address within the FDE where the CIE pointer is stored and the
 	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2069,8 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       if (fde->cie == NULL)
 	{
 	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      EH_CIE_TYPE_ID);
 	  fde->cie = find_cie (cie_table, cie_pointer);
 	}
 
@@ -2089,11 +2111,14 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   return end;
 }
 
-/* Read a CIE or FDE in BUF and decode it.  */
+/* Read a CIE or FDE in BUF and decode it. Entry_type specifies whether we
+   expect an FDE or a CIE.  */
+
 static gdb_byte *
 decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                     struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
 {
   enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
   gdb_byte *ret;
@@ -2102,7 +2127,7 @@ decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   while (1)
     {
       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+				  cie_table, fde_table, entry_type);
       if (ret != NULL)
 	break;
 
@@ -2212,6 +2237,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
   struct dwarf2_cie_table cie_table;
   struct dwarf2_fde_table fde_table;
   struct dwarf2_fde_table *fde_table2;
+  volatile struct gdb_exception e;
 
   cie_table.num_entries = 0;
   cie_table.entries = NULL;
@@ -2253,10 +2279,28 @@ dwarf2_build_frame_info (struct objfile *objfile)
           if (txt)
             unit->tbase = txt->vma;
 
-          frame_ptr = unit->dwarf_frame_buffer;
-          while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-            frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+	  TRY_CATCH (e, RETURN_MASK_ERROR)
+	    {
+	      frame_ptr = unit->dwarf_frame_buffer;
+	      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+		frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
+						&cie_table, &fde_table,
+						EH_CIE_OR_FDE_TYPE_ID);
+	    }
+
+	  if (e.reason < 0)
+	    {
+	      warning (_("skipping .eh_frame info of %s: %s"),
+		       objfile->name, e.message);
+
+	      if (fde_table.num_entries != 0)
+		{
+		  xfree(fde_table.entries);
+		  fde_table.entries = NULL;
+		  fde_table.num_entries = 0;
+		}
+	      /* The cie_table is discarded by the next if.  */
+	    }
 
           if (cie_table.num_entries != 0)
             {
@@ -2274,10 +2318,39 @@ dwarf2_build_frame_info (struct objfile *objfile)
                            &unit->dwarf_frame_size);
   if (unit->dwarf_frame_size)
     {
-      frame_ptr = unit->dwarf_frame_buffer;
-      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-	frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
-                                        &cie_table, &fde_table);
+      int num_old_fde_entries = fde_table.num_entries;
+
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  frame_ptr = unit->dwarf_frame_buffer;
+	  while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+	    frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
+					    &cie_table, &fde_table,
+					    EH_CIE_OR_FDE_TYPE_ID);
+	}
+      if (e.reason < 0)
+	{
+	  warning (_("skipping .debug_frame info of %s: %s"),
+		   objfile->name, e.message);
+
+	  if (fde_table.num_entries != 0)
+	    {
+	      fde_table.num_entries = num_old_fde_entries;
+	      if (num_old_fde_entries == 0)
+		{
+		  xfree (fde_table.entries);
+		  fde_table.entries = NULL;
+		}
+	      else
+		{
+		    /* just shrinks, could be skipped */
+		    xrealloc (fde_table.entries,fde_table.num_entries *
+			      sizeof (fde_table.entries[0]));
+		}
+	    }
+	  fde_table.num_entries = num_old_fde_entries;
+	  /* The cie_table is discarded by the next if.  */
+	}
     }
 
   /* Discard the cie_table, it is no longer needed.  */

Re: [PATCH,gdb,Update6]: ensures that cie ptr of an fda is a cie

[Tom Tromey]

>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:

Fawzi> I added a comment about that.

Better to just use the result.

Fawzi> +		{
Fawzi> +		    /* just shrinks, could be skipped */
Fawzi> +		    xrealloc (fde_table.entries,fde_table.num_entries *
Fawzi> +			      sizeof (fde_table.entries[0]));

Still the wrong indentation here.

Comments must start with an upper-case letter and end with a period and
two spaces.

Tom

Re: [PATCH,gdb,Update7]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

On Jul 11, 2011, at 9:49 PM, ext Tom Tromey wrote:

>>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:
> 
> Fawzi> I added a comment about that.
> 
> Better to just use the result.

done

2011-07-07  Fawzi Mohamed  <fawzi.mohamed@nokia.com>

	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
	that CIE pointer of an FDE really points to a CIE .

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index b68a773..964490c 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1801,17 +1801,30 @@ add_fde (struct dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
 #define DW64_CIE_ID ~0
 #endif
 
+/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
+   or any of them.  */
+
+enum eh_frame_type
+{
+  EH_CIE_TYPE_ID = 1 << 0,
+  EH_FDE_TYPE_ID = 1 << 1,
+  EH_CIE_OR_FDE_TYPE_ID = EH_CIE_TYPE_ID | EH_FDE_TYPE_ID
+};
+
 static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
 				     int eh_frame_p,
                                      struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
+
+/* Decode the next CIE or FDE, entry_type specifies the expected type.
+   Return NULL if invalid input, otherwise the next byte to be processed.  */
 
-/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
-   the next byte to be processed.  */
 static gdb_byte *
 decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                       struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   gdb_byte *buf, *end;
@@ -1862,6 +1875,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       char *augmentation;
       unsigned int cie_version;
 
+      /* Check that a CIE was expected.  */
+      if ((entry_type & EH_CIE_TYPE_ID) == 0)
+	error (_("Found a CIE when not expecting it."));
+
       /* Record the offset into the .debug_frame section of this CIE.  */
       cie_pointer = start - unit->dwarf_frame_buffer;
 
@@ -2027,6 +2044,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       /* This is a FDE.  */
       struct dwarf2_fde *fde;
 
+      /* Check that an FDE was expected.  */
+      if ((entry_type & EH_FDE_TYPE_ID) == 0)
+	error (_("Found an FDE when not expecting it."));
+
       /* In an .eh_frame section, the CIE pointer is the delta between the
 	 address within the FDE where the CIE pointer is stored and the
 	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2069,8 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       if (fde->cie == NULL)
 	{
 	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      EH_CIE_TYPE_ID);
 	  fde->cie = find_cie (cie_table, cie_pointer);
 	}
 
@@ -2089,11 +2111,14 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   return end;
 }
 
-/* Read a CIE or FDE in BUF and decode it.  */
+/* Read a CIE or FDE in BUF and decode it. Entry_type specifies whether we
+   expect an FDE or a CIE.  */
+
 static gdb_byte *
 decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                     struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
 {
   enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
   gdb_byte *ret;
@@ -2102,7 +2127,7 @@ decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   while (1)
     {
       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+				  cie_table, fde_table, entry_type);
       if (ret != NULL)
 	break;
 
@@ -2212,6 +2237,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
   struct dwarf2_cie_table cie_table;
   struct dwarf2_fde_table fde_table;
   struct dwarf2_fde_table *fde_table2;
+  volatile struct gdb_exception e;
 
   cie_table.num_entries = 0;
   cie_table.entries = NULL;
@@ -2253,10 +2279,28 @@ dwarf2_build_frame_info (struct objfile *objfile)
           if (txt)
             unit->tbase = txt->vma;
 
-          frame_ptr = unit->dwarf_frame_buffer;
-          while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-            frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+	  TRY_CATCH (e, RETURN_MASK_ERROR)
+	    {
+	      frame_ptr = unit->dwarf_frame_buffer;
+	      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+		frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
+						&cie_table, &fde_table,
+						EH_CIE_OR_FDE_TYPE_ID);
+	    }
+
+	  if (e.reason < 0)
+	    {
+	      warning (_("skipping .eh_frame info of %s: %s"),
+		       objfile->name, e.message);
+
+	      if (fde_table.num_entries != 0)
+		{
+		  xfree(fde_table.entries);
+		  fde_table.entries = NULL;
+		  fde_table.num_entries = 0;
+		}
+	      /* The cie_table is discarded by the next if.  */
+	    }
 
           if (cie_table.num_entries != 0)
             {
@@ -2274,10 +2318,39 @@ dwarf2_build_frame_info (struct objfile *objfile)
                            &unit->dwarf_frame_size);
   if (unit->dwarf_frame_size)
     {
-      frame_ptr = unit->dwarf_frame_buffer;
-      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-	frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
-                                        &cie_table, &fde_table);
+      int num_old_fde_entries = fde_table.num_entries;
+
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  frame_ptr = unit->dwarf_frame_buffer;
+	  while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+	    frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
+					    &cie_table, &fde_table,
+					    EH_CIE_OR_FDE_TYPE_ID);
+	}
+      if (e.reason < 0)
+	{
+	  warning (_("skipping .debug_frame info of %s: %s"),
+		   objfile->name, e.message);
+
+	  if (fde_table.num_entries != 0)
+	    {
+	      fde_table.num_entries = num_old_fde_entries;
+	      if (num_old_fde_entries == 0)
+		{
+		  xfree (fde_table.entries);
+		  fde_table.entries = NULL;
+		}
+	      else
+		{
+		  fde_table.entries = xrealloc (fde_table.entries,
+						fde_table.num_entries *
+						sizeof (fde_table.entries[0]));
+		}
+	    }
+	  fde_table.num_entries = num_old_fde_entries;
+	  /* The cie_table is discarded by the next if.  */
+	}
     }
 
   /* Discard the cie_table, it is no longer needed.  */

Re: [PATCH,gdb,Update7]: ensures that cie ptr of an fda is a cie

[Tom Tromey]

>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:

Fawzi> 2011-07-07  Fawzi Mohamed  <fawzi.mohamed@nokia.com>
Fawzi> 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
Fawzi> 	that CIE pointer of an FDE really points to a CIE .

Still one nit.

Fawzi> +		  xfree(fde_table.entries);

Space before the open paren.

This is ok with this change.

If you do not have a sourceware account, let me know and I will get you
set up with one.

Please also send and commit a patch to add yourself to the
write-after-approval section of gdb/MAINTAINERS.

thanks,
Tom

FYI [PATCH,gdb,Update7]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

On Jul 12, 2011, at 9:50 PM, ext Tom Tromey wrote:

>>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:
> 
> Fawzi> 2011-07-07  Fawzi Mohamed  <fawzi.mohamed@nokia.com>
> Fawzi> 	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
> Fawzi> 	that CIE pointer of an FDE really points to a CIE .
> [...]
> This is ok with this change.
> [...]

thanks checked in

Fawzi

Re: [PATCH,gdb,Update5]: ensures that cie ptr of an fda is a cie

[Fawzi Mohamed]

Clearly as soon as I sent it away I realized that I did not check if the arguments of
printf were null. Here a new patch reviewed by Andre, hopefully close to ok :)

master is still a bit buggy on mac, and I have found that 10.7 lion uses some more load commands
in mach-o that are not even defined in the mach-o/loader.h, but that is something for another time
(I will switch back to the 7.2 branch for now).

Fawzi

2011-07-07  Fawzi Mohamed  <fawzi.mohamed@nokia.com>

	* dwarf2-frame.c (decode_frame_entry, decode_frame_entry_1): Ensure
	that CIE pointer of an FDE really points to a CIE .

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index b68a773..dcc3682 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1801,17 +1801,30 @@ add_fde (struct dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
 #define DW64_CIE_ID ~0
 #endif
 
+/* Defines the type of eh_frames that are expected to be decoded: CIE, FDE
+   or any of them.  */
+
+enum eh_frame_type
+{
+  EH_CIE_TYPE_ID = 1 << 0,
+  EH_FDE_TYPE_ID = 1 << 1,
+  EH_CIE_OR_FDE_TYPE_ID = EH_CIE_TYPE_ID | EH_FDE_TYPE_ID
+};
+
 static gdb_byte *decode_frame_entry (struct comp_unit *unit, gdb_byte *start,
 				     int eh_frame_p,
                                      struct dwarf2_cie_table *cie_table,
-                                     struct dwarf2_fde_table *fde_table);
+                                     struct dwarf2_fde_table *fde_table,
+                                     enum eh_frame_type entry_type);
+
+/* Decode the next CIE or FDE, entry_type specifies the expected type.
+   Return NULL if invalid input, otherwise the next byte to be processed.  */
 
-/* Decode the next CIE or FDE.  Return NULL if invalid input, otherwise
-   the next byte to be processed.  */
 static gdb_byte *
 decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                       struct dwarf2_cie_table *cie_table,
-                      struct dwarf2_fde_table *fde_table)
+                      struct dwarf2_fde_table *fde_table,
+                      enum eh_frame_type entry_type)
 {
   struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
   gdb_byte *buf, *end;
@@ -1862,6 +1875,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       char *augmentation;
       unsigned int cie_version;
 
+      /* Check that a CIE was expected.  */
+      if ((entry_type & EH_CIE_TYPE_ID) == 0)
+	error (_("Found a CIE when not expecting it, broken eh_info"));
+
       /* Record the offset into the .debug_frame section of this CIE.  */
       cie_pointer = start - unit->dwarf_frame_buffer;
 
@@ -2027,6 +2044,10 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       /* This is a FDE.  */
       struct dwarf2_fde *fde;
 
+      /* Check that an FDE was expected.  */
+      if ((entry_type & EH_FDE_TYPE_ID) == 0)
+	error (_("Found an FDE when not expecting it, broken eh_info."));
+
       /* In an .eh_frame section, the CIE pointer is the delta between the
 	 address within the FDE where the CIE pointer is stored and the
 	 address of the CIE.  Convert it to an offset into the .eh_frame
@@ -2048,7 +2069,8 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
       if (fde->cie == NULL)
 	{
 	  decode_frame_entry (unit, unit->dwarf_frame_buffer + cie_pointer,
-			      eh_frame_p, cie_table, fde_table);
+			      eh_frame_p, cie_table, fde_table,
+			      EH_CIE_TYPE_ID);
 	  fde->cie = find_cie (cie_table, cie_pointer);
 	}
 
@@ -2089,11 +2111,14 @@ decode_frame_entry_1 (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   return end;
 }
 
-/* Read a CIE or FDE in BUF and decode it.  */
+/* Read a CIE or FDE in BUF and decode it. Entry_type specifies whether we
+   expect an FDE or a CIE.  */
+
 static gdb_byte *
 decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
                     struct dwarf2_cie_table *cie_table,
-                    struct dwarf2_fde_table *fde_table)
+                    struct dwarf2_fde_table *fde_table,
+                    enum eh_frame_type entry_type)
 {
   enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
   gdb_byte *ret;
@@ -2102,7 +2127,7 @@ decode_frame_entry (struct comp_unit *unit, gdb_byte *start, int eh_frame_p,
   while (1)
     {
       ret = decode_frame_entry_1 (unit, start, eh_frame_p,
-                                  cie_table, fde_table);
+				  cie_table, fde_table, entry_type);
       if (ret != NULL)
 	break;
 
@@ -2212,6 +2237,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
   struct dwarf2_cie_table cie_table;
   struct dwarf2_fde_table fde_table;
   struct dwarf2_fde_table *fde_table2;
+  volatile struct gdb_exception e;
 
   cie_table.num_entries = 0;
   cie_table.entries = NULL;
@@ -2253,10 +2279,29 @@ dwarf2_build_frame_info (struct objfile *objfile)
           if (txt)
             unit->tbase = txt->vma;
 
-          frame_ptr = unit->dwarf_frame_buffer;
-          while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-            frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
-                                            &cie_table, &fde_table);
+	  TRY_CATCH (e, RETURN_MASK_ERROR)
+	    {
+	      frame_ptr = unit->dwarf_frame_buffer;
+	      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+		frame_ptr = decode_frame_entry (unit, frame_ptr, 1,
+						&cie_table, &fde_table,
+						EH_CIE_OR_FDE_TYPE_ID);
+	    }
+
+	  if (e.reason < 0)
+	    {
+	      warning (_("skipping .eh_frame info of %s: %s"),
+		       (objfile->name ? objfile->name : "*unknown*"),
+		       (e.message ? e.message : ""));
+
+	      if (fde_table.num_entries != 0)
+		{
+		  xfree(fde_table.entries);
+		  fde_table.entries = NULL;
+		  fde_table.num_entries = 0;
+		}
+	      /* The cie_table is discarded by the next if.  */
+	    }
 
           if (cie_table.num_entries != 0)
             {
@@ -2274,10 +2319,39 @@ dwarf2_build_frame_info (struct objfile *objfile)
                            &unit->dwarf_frame_size);
   if (unit->dwarf_frame_size)
     {
-      frame_ptr = unit->dwarf_frame_buffer;
-      while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
-	frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
-                                        &cie_table, &fde_table);
+      int num_old_fde_entries = fde_table.num_entries;
+
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  frame_ptr = unit->dwarf_frame_buffer;
+	  while (frame_ptr < unit->dwarf_frame_buffer + unit->dwarf_frame_size)
+	    frame_ptr = decode_frame_entry (unit, frame_ptr, 0,
+					    &cie_table, &fde_table,
+					    EH_CIE_OR_FDE_TYPE_ID);
+	}
+      if (e.reason < 0)
+	{
+	  warning (_("skipping .debug_frame info of %s: %s"),
+		   (objfile->name ? objfile->name : "*unknown*"),
+		   (e.message ? e.message : ""));
+
+	  if (fde_table.num_entries != 0)
+	    {
+	      fde_table.num_entries = num_old_fde_entries;
+	      if (num_old_fde_entries == 0)
+		{
+		  xfree(fde_table.entries);
+		  fde_table.entries = NULL;
+		}
+	      else
+		{
+		    xrealloc (fde_table.entries,
+			      fde_table.num_entries * sizeof (fde_table.entries[0]));
+		}
+	    }
+	  fde_table.num_entries = num_old_fde_entries;
+	  /* The cie_table is discarded by the next if.  */
+	}
     }
 
   /* Discard the cie_table, it is no longer needed.  */

Re: [PATCH,gdb,Update5]: ensures that cie ptr of an fda is a cie

[Tom Tromey]

>>>>> "Fawzi" == Fawzi Mohamed <fawzi.mohamed@nokia.com> writes:

Fawzi> Clearly as soon as I sent it away I realized that I did not check
Fawzi> if the arguments of printf were null.

I appreciate your attention to detail, but I don't think these checks
are needed:

Fawzi> +	      warning (_("skipping .eh_frame info of %s: %s"),
Fawzi> +		       (objfile->name ? objfile->name : "*unknown*"),

objfile->name cannot be NULL.
This is assumed generally.

Fawzi> +		       (e.message ? e.message : ""));

Likewise for the exception.

Tom