[PATCH] gdb: objc-lang: check symbol name before accessing memory

[PATCH] gdb: objc-lang: check symbol name before accessing memory

[Mike Frysinger]

The current ObjC logic will check both the symbol name and the target
address space when trying to locate an appropriate selector.  The problem
is that first the target address space is checked before the symbol name.
This may lead to a lot of unnecessary host<->target transactions when
dealing with a non-OjbC target that does use function descriptors to
describe functions as every symbol will have its FD read just to have the
result thrown away with non-matching symbol names.

It also may lead to problems when a non-FD symbol is found that points near
the end of the address space as the target will throw up a memory_error().
One such example are symbols that are not functions, smaller than a FD,
and are the last valid location.  Obviously treating it as a larger data
struct can cause memory overflows.

So to speed things up and not screw over such targets, check the symbol
name (which we already have locally) first before attempting to read the
function's descriptor.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2010-04-13  Mike Frysinger  <vapier@gentoo.org>

	* objc-lang.c (find_methods): Move symname check up.
---
 gdb/objc-lang.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index a050f15..8e1de75 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -1178,6 +1178,14 @@ find_methods (struct symtab *symtab, char type,
 
 	  QUIT;
 
+	  symname = SYMBOL_NATURAL_NAME (msymbol);
+	  if (symname == NULL)
+	    continue;
+
+	  if ((symname[0] != '-' && symname[0] != '+') || (symname[1] != '['))
+	    /* Not a method name.  */
+	    continue;
+
 	  /* The minimal symbol might point to a function descriptor;
 	     resolve it to the actual code address instead.  */
 	  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
@@ -1188,14 +1196,7 @@ find_methods (struct symtab *symtab, char type,
 	      /* Not in the specified symtab.  */
 	      continue;
 
-	  symname = SYMBOL_NATURAL_NAME (msymbol);
-	  if (symname == NULL)
-	    continue;
-
-	  if ((symname[0] != '-' && symname[0] != '+') || (symname[1] != '['))
-	    /* Not a method name.  */
-	    continue;
-      
+	  /* Now that thinks are a bit sane, clean up the symname.  */
 	  while ((strlen (symname) + 1) >= tmplen)
 	    {
 	      tmplen = (tmplen == 0) ? 1024 : tmplen * 2;
-- 
1.7.0.2

Re: [PATCH] gdb: objc-lang: check symbol name before accessing memory

[Joel Brobecker]

> 2010-04-13  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* objc-lang.c (find_methods): Move symname check up.

This is OK, but I would like you to add a comment before the symbol name
check explaining that this check is performed first because it can be
performed entirely without sending any query to the target.

Can you also confirm how you tested this patch, please?

-- 
Joel

Re: [PATCH] gdb: objc-lang: check symbol name before accessing memory

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 746 bytes --]

On Monday 19 April 2010 10:55:03 Joel Brobecker wrote:
> > 2010-04-13  Mike Frysinger  <vapier@gentoo.org>
> > 
> > 	* objc-lang.c (find_methods): Move symname check up.
> 
> This is OK, but I would like you to add a comment before the symbol name
> check explaining that this check is performed first because it can be
> performed entirely without sending any query to the target.

OK

> Can you also confirm how you tested this patch, please?

i'm testing a Blackfin target via gdbserver.  trying to set a breakpoint with 
FDPIC always hit memory_error() before the change while it worked correctly 
after the change.  wrt ObjC, i dont really know anything about the language, 
so i cant really explicitly test that ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gdb: objc-lang: check symbol name before accessing memory

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 462 bytes --]

On Monday 19 April 2010 10:55:03 Joel Brobecker wrote:
> Can you also confirm how you tested this patch, please?

i just did `make check-gdb` and looks regression free.  there's a few tests 
that look like they're picky and can go from pass to fail based on the wind.

before: 15280 pass / 187 fail
after: 15282 pass / 186 fail

as this change didnt cause any native regressions either on x86-64/linux, i'm 
committing w/your suggested change.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]