[PATCH] TUI: Fix buffer overflow in tui_expand_tabs

[PATCH] TUI: Fix buffer overflow in tui_expand_tabs

[Anton Blanchard]

tui_expand_tabs writes past the end of the buffers it allocates
because we forget to zero out col. This results in us adding more
spaces than we need to get aligned, and we write past the end of the
allocated buffer.

This was noticed on Ubuntu Vivid ppc64le, where gdb would SEGV when
using the TUI.

2015-03-17  Anton Blanchard  <anton@samba.org>

gdb/ChangeLog:
	* tui/tui-io.c (tui_expand_tabs): Zero col before reusing.
---
 gdb/ChangeLog    | 4 ++++
 gdb/tui/tui-io.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d984565..4e0177a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+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)
-- 
2.1.0

Re: [PATCH] TUI: Fix buffer overflow in tui_expand_tabs

[Anton Blanchard]

Hi,

> tui_expand_tabs writes past the end of the buffers it allocates
> because we forget to zero out col. This results in us adding more
> spaces than we need to get aligned, and we write past the end of the
> allocated buffer.
> 
> This was noticed on Ubuntu Vivid ppc64le, where gdb would SEGV when
> using the TUI.

Any feedback on this? We either need to fix it, or back out commit
312809f88389 ("Make sure TABs are expanded in TUI windows on
MS-Windows.")

Anton

> 2015-03-17  Anton Blanchard  <anton@samba.org>
> 
> gdb/ChangeLog:
> 	* tui/tui-io.c (tui_expand_tabs): Zero col before reusing.
> ---
>  gdb/ChangeLog    | 4 ++++
>  gdb/tui/tui-io.c | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index d984565..4e0177a 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +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)

Re: [PATCH] TUI: Fix buffer overflow in tui_expand_tabs

[Doug Evans]

On Mon, Mar 16, 2015 at 4:30 PM, Anton Blanchard <anton@samba.org> wrote:
> tui_expand_tabs writes past the end of the buffers it allocates
> because we forget to zero out col. This results in us adding more
> spaces than we need to get aligned, and we write past the end of the
> allocated buffer.
>
> This was noticed on Ubuntu Vivid ppc64le, where gdb would SEGV when
> using the TUI.
>
> 2015-03-17  Anton Blanchard  <anton@samba.org>
>
> gdb/ChangeLog:
>         * tui/tui-io.c (tui_expand_tabs): Zero col before reusing.
> ---
>  gdb/ChangeLog    | 4 ++++
>  gdb/tui/tui-io.c | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index d984565..4e0177a 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +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.

Re: [PATCH] TUI: Fix buffer overflow in tui_expand_tabs

[Eli Zaretskii]

> 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; )
@@ -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

Re: [PATCH] TUI: Fix buffer overflow in tui_expand_tabs

[Doug Evans]

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!

Re: [PATCH] TUI: Fix buffer overflow in tui_expand_tabs

[Eli Zaretskii]

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