TUI: Fix buffer overflow in tui_expand_tabs
Commit Message
> Date: Thu, 19 Mar 2015 15:57:58 -0700
> From: Doug Evans <dje@google.com>
> Cc: gdb-patches <gdb-patches@sourceware.org>, Eli Zaretskii <eliz@gnu.org>
>
> > +2015-03-17 Anton Blanchard <anton@samba.org>
> > +
> > + * tui/tui-io.c (tui_expand_tabs): Zero col before reusing.
> > +
> > 2015-03-16 John Baldwin <jhb@FreeBSD.org>
> >
> > * fbsd-tdep.c (fbsd_make_corefile_notes): Fetch all target registers
> > diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
> > index a8af9b6..02ae17d 100644
> > --- a/gdb/tui/tui-io.c
> > +++ b/gdb/tui/tui-io.c
> > @@ -690,7 +690,7 @@ tui_expand_tabs (const char *string, int col)
> > ret = q = xmalloc (strlen (string) + n_adjust + 1);
> >
> > /* 2. Copy the original string while replacing TABs with spaces. */
> > - for (s = string; s; )
> > + for (col = 0, s = string; s; )
> > {
> > char *s1 = strpbrk (s, "\t");
> > if (s1)
>
> Hi.
>
> col needs to be reset to its original value on function entry, right?
> I suggest changing the code so that col is left unmodified,
> and use a new variable to track the advance of col in both loops.
Sorry about the bug. Does the below look correct?
Comments
Eli Zaretskii writes:
> > Date: Thu, 19 Mar 2015 15:57:58 -0700
> > From: Doug Evans <dje@google.com>
> > Cc: gdb-patches <gdb-patches@sourceware.org>, Eli Zaretskii <eliz@gnu.org>
> >
> > > +2015-03-17 Anton Blanchard <anton@samba.org>
> > > +
> > > + * tui/tui-io.c (tui_expand_tabs): Zero col before reusing.
> > > +
> > > 2015-03-16 John Baldwin <jhb@FreeBSD.org>
> > >
> > > * fbsd-tdep.c (fbsd_make_corefile_notes): Fetch all target registers
> > > diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
> > > index a8af9b6..02ae17d 100644
> > > --- a/gdb/tui/tui-io.c
> > > +++ b/gdb/tui/tui-io.c
> > > @@ -690,7 +690,7 @@ tui_expand_tabs (const char *string, int col)
> > > ret = q = xmalloc (strlen (string) + n_adjust + 1);
> > >
> > > /* 2. Copy the original string while replacing TABs with spaces. */
> > > - for (s = string; s; )
> > > + for (col = 0, s = string; s; )
> > > {
> > > char *s1 = strpbrk (s, "\t");
> > > if (s1)
> >
> > Hi.
> >
> > col needs to be reset to its original value on function entry, right?
> > I suggest changing the code so that col is left unmodified,
> > and use a new variable to track the advance of col in both loops.
>
> Sorry about the bug. Does the below look correct?
>
> --- gdb/tui/tui-io.c~ 2015-02-20 19:11:44.000000000 +0200
> +++ gdb/tui/tui-io.c 2015-03-20 10:01:11.289375000 +0200
> @@ -761,6 +761,7 @@ tui_expand_tabs (const char *string, int
> int n_adjust;
> const char *s;
> char *ret, *q;
> + int nc = col;
>
> /* 1. How many additional characters do we need? */
> for (n_adjust = 0, s = string; s; )
Nit. To my eyes the following would be more readable.
- int n_adjust;
+ int nc, n_adjust;
const char *s;
char *ret, *q;
/* 1. How many additional characters do we need? */
- for (n_adjust = 0, s = string; s; )
+ for (nc = col, n_adjust = 0, s = string; s; )
> @@ -768,10 +769,10 @@ tui_expand_tabs (const char *string, int
> s = strpbrk (s, "\t");
> if (s)
> {
> - col += (s - string) + n_adjust;
> + nc += (s - string) + n_adjust;
> /* Adjustment for the next tab stop, minus one for the TAB
> we replace with spaces. */
> - n_adjust += 8 - (col % 8) - 1;
> + n_adjust += 8 - (nc % 8) - 1;
> s++;
> }
> }
> @@ -780,7 +781,7 @@ tui_expand_tabs (const char *string, int
> ret = q = xmalloc (strlen (string) + n_adjust + 1);
>
> /* 2. Copy the original string while replacing TABs with spaces. */
> - for (s = string; s; )
> + for (s = string, nc = col; s; )
> {
> char *s1 = strpbrk (s, "\t");
> if (s1)
> @@ -789,12 +790,12 @@ tui_expand_tabs (const char *string, int
> {
> strncpy (q, s, s1 - s);
> q += s1 - s;
> - col += s1 - s;
> + nc += s1 - s;
> }
> do {
> *q++ = ' ';
> - col++;
> - } while ((col % 8) != 0);
> + nc++;
> + } while ((nc % 8) != 0);
> s1++;
> }
> else
Ok with that change.
Thanks!
> From: Doug Evans <dje@google.com>
> Date: Fri, 20 Mar 2015 09:24:06 -0700
> Cc: anton@samba.org,
> gdb-patches@sourceware.org
>
> Nit. To my eyes the following would be more readable.
>
> - int n_adjust;
> + int nc, n_adjust;
> const char *s;
> char *ret, *q;
>
> /* 1. How many additional characters do we need? */
> - for (n_adjust = 0, s = string; s; )
> + for (nc = col, n_adjust = 0, s = string; s; )
Thanks, pushed with that change (master and gdb-7.9-branch).
@@ -761,6 +761,7 @@ tui_expand_tabs (const char *string, int
int n_adjust;
const char *s;
char *ret, *q;
+ int nc = col;
/* 1. How many additional characters do we need? */
for (n_adjust = 0, s = string; s; )
@@ -768,10 +769,10 @@ tui_expand_tabs (const char *string, int
s = strpbrk (s, "\t");
if (s)
{
- col += (s - string) + n_adjust;
+ nc += (s - string) + n_adjust;
/* Adjustment for the next tab stop, minus one for the TAB
we replace with spaces. */
- n_adjust += 8 - (col % 8) - 1;
+ n_adjust += 8 - (nc % 8) - 1;
s++;
}
}
@@ -780,7 +781,7 @@ tui_expand_tabs (const char *string, int
ret = q = xmalloc (strlen (string) + n_adjust + 1);
/* 2. Copy the original string while replacing TABs with spaces. */
- for (s = string; s; )
+ for (s = string, nc = col; s; )
{
char *s1 = strpbrk (s, "\t");
if (s1)
@@ -789,12 +790,12 @@ tui_expand_tabs (const char *string, int
{
strncpy (q, s, s1 - s);
q += s1 - s;
- col += s1 - s;
+ nc += s1 - s;
}
do {
*q++ = ' ';
- col++;
- } while ((col % 8) != 0);
+ nc++;
+ } while ((nc % 8) != 0);
s1++;
}
else