This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFA2] Follow-up decode_line_1 crash
- To: Keith Seitz <keiths at cygnus dot com>
- Subject: Re: [RFA2] Follow-up decode_line_1 crash
- From: "Martin M. Hunt" <hunt at redhat dot com>
- Date: Wed, 14 Mar 2001 12:32:20 -0800 (PST)
- cc: <gdb-patches at sources dot redhat dot com>
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
>
>