[RFA2] Follow-up decode_line_1 crash

[RFA2] Follow-up decode_line_1 crash

[Keith Seitz]

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

Re: [RFA2] Follow-up decode_line_1 crash

[Fernando Nasser]

I have tried it and it looks OK (on Red Hat 7.0).

Let's try it.

Fernando


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

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA2] Follow-up decode_line_1 crash

[Keith Seitz]

> > Patch:
> > 
[snip]

I forgot to post the ChangeLog (which probably requires comments):

2001-03-14  Keith Seitz  <keiths@cygnus.com>

	* linespec.c (decode_line_1): Skip argptr over a leading
	double quote. Prevents alloc of 0 bytes and memcpy of -1 bytes.

Keith

Re: [RFA2] Follow-up decode_line_1 crash

[Fernando Nasser]

Keith Seitz wrote:
> 
> > > Patch:
> > >
> [snip]
> 
> I forgot to post the ChangeLog (which probably requires comments):
> 
> 2001-03-14  Keith Seitz  <keiths@cygnus.com>
> 
>         * linespec.c (decode_line_1): Skip argptr over a leading
>         double quote. Prevents alloc of 0 bytes and memcpy of -1 bytes.
> 

Sounds good.

Approved.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA2] Follow-up decode_line_1 crash

[Keith Seitz]

On Wed, 14 Mar 2001, Fernando Nasser wrote:

> I have tried it and it looks OK (on Red Hat 7.0).
> 
> Let's try it.

I have checked in this patch.

Keith

Re: [RFA2] Follow-up decode_line_1 crash

[Martin M. Hunt]

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
>
>

Re: [RFA2] Follow-up decode_line_1 crash

[Eli Zaretskii]

On Wed, 14 Mar 2001, Keith Seitz wrote:

> *** 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;

Can the argument have embedded (escaped) quotes, as in "foo\"bar", in
some language?  If it can, the search for the next quote is certainly
not the right thing to do.

One situation where this could happen is when a breakpoint uses the 
file:line notation, since file names could have embedded quotes.

Re: [RFA2] Follow-up decode_line_1 crash

[Fernando Nasser]

Martin,

Please check this in as an immediate remedy.

We still have to address Eli's issue with quoted '"' that may appear in
filenames (can they?).  

An improvement would be to check the previous character and if it is a
'\' do not consider that quote as a second quote.

P.S.: I am adding the need of a "testsuite/gdb.base/linespec.exp" and a
"testsuite/gdb.c++/linespec2.exp" to the TODO file.



"Martin M. Hunt" wrote:
> 
> 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
> >
> >

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA2] Follow-up decode_line_1 crash

[Eli Zaretskii]

On Thu, 15 Mar 2001, Fernando Nasser wrote:

> We still have to address Eli's issue with quoted '"' that may appear in
> filenames (can they?).

On Unix, sure they can.  DOS and Windows specifically disallow " from 
appearing in a file name.

I agree that plumbing the immediate bug, even without resoolving the case 
of '"', is the right thing to do, because a  quote in a file name is a 
very rare case, even more rare than a space.

Re: [RFA2] Follow-up decode_line_1 crash

[Martin M. Hunt]

OK.  Checked in now.

On Thu, 15 Mar 2001, Fernando Nasser wrote:

> Martin,
>
> Please check this in as an immediate remedy.
>
> We still have to address Eli's issue with quoted '"' that may appear in
> filenames (can they?).
>
> An improvement would be to check the previous character and if it is a
> '\' do not consider that quote as a second quote.
>
> P.S.: I am adding the need of a "testsuite/gdb.base/linespec.exp" and a
> "testsuite/gdb.c++/linespec2.exp" to the TODO file.
>
>
>
> "Martin M. Hunt" wrote:
> >
> > 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
> > >
> > >
>
> --
> Fernando Nasser
> Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
> 2323 Yonge Street, Suite #300
> Toronto, Ontario   M4P 2C9
>