[RFA2] Follow-up decode_line_1 crash

Martin M. Hunt hunt@redhat.com
Wed Mar 14 12:32:00 GMT 2001


This is really not my area, but I happened to try to decypher this file
a few weeks ago.  Its full of surprises.

for example, starting on line 622, we have

for (; *p; p++)
{
  parse stuff ...
}

/* if the closing double quote was left at the end, remove it */
  if (is_quote_enclosed)
    {
      char *closing_quote = strchr (p, '"');
      if (closing_quote && closing_quote[1] == '\0')
	*closing_quote = '\0';
    }

so the for loop parses things until *p == NULL, then looks for the closing
quote starting at the NULL?!??

Keith's patch changes 2 things.  The first change makes no difference.
The second change is to increment the start of the parsed string, which is
certainly right. I think it would be better to leave the p++ in.
       (*argptr)++;
	p++;

I tried fixing the strchr to actually remove the trailing quote and had no
test failures on Linux.  It is quite possible it messes up something
unexpected.

Current (with Keith's patch)
(top-gdb) b "main"
Junk at end of arguments.
(top-gdb) b "foo bar.c:602"
No source file named foo bar.c.
(top-gdb) b ""
Segmentation fault

With the following patch
(top-gdb) b "main"
Breakpoint 3 at 0x80832aa: file ../../src/gdb/main.c, line 764.
(top-gdb) b "foo bar.c:602"
No source file named foo bar.c.
(top-gdb) b ""
Function "" not defined.
(top-gdb)


Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.6
diff -p -r1.6 linespec.c
*** linespec.c	2001/03/14 18:36:45	1.6
--- linespec.c	2001/03/14 20:15:08
*************** decode_line_1 (char **argptr, int funfir
*** 612,621 ****

    s = NULL;
    p = *argptr;
!   if (**argptr == '"')
      {
        is_quote_enclosed = 1;
        (*argptr)++;
      }
    else
      is_quote_enclosed = 0;
--- 612,622 ----

    s = NULL;
    p = *argptr;
!   if (p[0] == '"')
      {
        is_quote_enclosed = 1;
        (*argptr)++;
+       p++;
      }
    else
      is_quote_enclosed = 0;
*************** decode_line_1 (char **argptr, int funfir
*** 654,660 ****
    /* if the closing double quote was left at the end, remove it */
    if (is_quote_enclosed)
      {
!       char *closing_quote = strchr (p, '"');
        if (closing_quote && closing_quote[1] == '\0')
  	*closing_quote = '\0';
      }
--- 655,661 ----
    /* if the closing double quote was left at the end, remove it */
    if (is_quote_enclosed)
      {
!       char *closing_quote = strchr (p-1, '"');
        if (closing_quote && closing_quote[1] == '\0')
  	*closing_quote = '\0';
      }
*************** decode_line_1 (char **argptr, int funfir
*** 1091,1099 ****
      {
        p = skip_quoted (*argptr);
      }
-
-   if (is_quote_enclosed && **argptr == '"')
-     (*argptr)++;

    copy = (char *) alloca (p - *argptr + 1);
    memcpy (copy, *argptr, p - *argptr);
--- 1092,1097 ----


On Wed, 14 Mar 2001, Keith Seitz wrote:

>
> Problem:
>
> $ gdb -nw -nx -q
> (gdb) b "foo"
> Segmentation fault (core dumped)
>
> decode_linespec_1 does something like:
>
> char *p = *argptr; (the first quote in "foo")
> if (p == '"')
>   {
>     p++;
>     is_quote_enclosed = 1;
>   }
>
>   if (is_quote_enclosed)
>     {
>       char *closing_quote = strchr (p, '"');
>       if (closing_quote && closing_quote[1] == '\0')
> 	*closing_quote = '\0';
>     }
>
> /* so now p looks like foo with no quotes and *argptr is "foo */
>
>
> char *copy = (char *) alloca (p - *argptr + 1); <-- alloca of 0 bytes
> memcpy (copy, *argptr, p - *argptr); <-- copy -1 bytes
>
> Patch:
>
> Index: linespec.c
> ===================================================================
> RCS file: /cvs/cvsfiles/devo/gdb/linespec.c,v
> retrieving revision 2.4
> diff -p -p -r2.4 linespec.c
> *** linespec.c	2000/12/20 14:34:15	2.4
> --- linespec.c	2001/03/14 16:16:11
> *************** decode_line_1 (char **argptr, int funfir
> *** 611,620 ****
>
>     s = NULL;
>     p = *argptr;
> !   if (p[0] == '"')
>       {
>         is_quote_enclosed = 1;
> !       p++;
>       }
>     else
>       is_quote_enclosed = 0;
> --- 611,620 ----
>
>     s = NULL;
>     p = *argptr;
> !   if (**argptr == '"')
>       {
>         is_quote_enclosed = 1;
> !       (*argptr)++;
>       }
>     else
>       is_quote_enclosed = 0;
>
> Tested on RH6.2. Should be generic enough to apply to all targets. I'm no
> expert at this stuff, but a crash is Just Plain Bad (TM).
>
> Keith
>
>




More information about the Gdb-patches mailing list