Re: [RFA2] Follow-up decode_line_1 crash

["Martin M. Hunt" <hunt at redhat dot com>]

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
>