Re: [RFA2] Follow-up decode_line_1 crash

[Fernando Nasser <fnasser@redhat.com>]

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