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

[Doug Evans <dje@google.com>]

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!