Minor off-by-one error in command_line_handler

Minor off-by-one error in command_line_handler

[Jason Molenda]

[-- Attachment #1: Type: text/plain, Size: 4004 bytes --]

I've started playing with Julian Seward's new tool, valgrind, which
finds a certain class of bugs very well.  It picked two out of gdb
right away, neither of which is particularly egrarious but one of
which should probably be fixed.

In command_line_handler() there is code like this ('rl' is a pointer
to the the line the user just entered):

  p1 = rl;
  /* Copy line.  Don't copy null at end.  (Leaves line alone
     if this was just a newline)  */
  while (*p1)
    *p++ = *p1++;

  xfree (rl);                   /* Allocated in readline.  */

  if (*(p - 1) == '\\')
    {


When rl is a zero-length string, i.e. the user hit RETURN alone,
p will point to the first byte of the allocated memory, aka rl[0],
so accessing (p - 1) reads the byte before the allocated string.

Besides the general idea that this isn't the best thing to be doing,
it's possible that the byte before the string could contain a '\'
value and cause odd behavior.  I'll attach a possible patch.

The other problem is with the ALL_BLOCK_SYMBOLS macro.  It looks
like this

/* Macro to loop through all symbols in a block BL.
   i counts which symbol we are looking at, and sym points to the current
   symbol.  */
#define ALL_BLOCK_SYMBOLS(bl, i, sym)                   \
        for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));    \
             (i) < BLOCK_NSYMS ((bl));                  \
             ++(i), (sym) = BLOCK_SYM ((bl), (i)))

Where the block structure (BL) ends with an array of pointers to
symbols.  The third expression in the for statement increments the
index variable and reads the address at the i'th element of the
bl->sym[] array.

So when a block has 2 symbols, bl->sym[0] and bl->sym[1] contain
values.  On the last evaluation of this loop, i is pre-incremented
from 1 to 2 and the statement 'sym = bl->nsym[2]' is done - we're
reading one element past the end of the array.

The invalid memory we just read is not used -- the conditional
expression is then evaluated and the loop exits.  The only way
I can see this causing a problem is on a system where reading
that unallocated word of memory would cause a segfault.  Unless
other people have heard complaints about gdb 5.1 doing so, I
don't think this is worth worrying about. 


FWIW valgrind is real nifty, despite being a bit 'fresh' in its
maturity.  Here's the home page
	http://www.muraroa.demon.co.uk/

Here's what its output looks like for the command_line_handler()
mistake:

(gdb) 
==32151== Invalid read of size 1
==32151==    at 0x80AA1BD: command_line_handler (../../src/gdb/event-top.c:684)
==32151==    by 0x81D365F: rl_callback_read_char (../../src/readline/callback.c:114)
==32151==    by 0x80A9443: rl_callback_read_char_wrapper (../../src/gdb/event-top.c:169)
==32151==    by 0x80A9BE4: stdin_event_handler (../../src/gdb/event-top.c:418)
==32151==    Address 0x4253C793 is 1 bytes before a block of size 80 alloc'd
==32151==    at 0x40065842: malloc (vg_clientmalloc.c:618)
==32151==    by 0x80EDEC9: mmalloc (../../src/gdb/utils.c:892)
==32151==    by 0x80EDFA2: xmmalloc (../../src/gdb/utils.c:1011)
==32151==    by 0x80EE0C7: xmalloc (../../src/gdb/utils.c:1083)


It has an option to drop you into gdb right at the point where the
bug occurs - you can step around in the inferior process and see
what it was up to.  Very nice.  valgrind works by simulating the
executable and tracking every read/write -- it can detect the use
of uninitialized memory or the use of inaccessible memory (e.g.
a malloc'ed block that was freed or memory on the stack of a
subroutine that is no longer valid.)  It has some facilities for
finding memory leaks, although I haven't started messing with those
yet.

It doesn't have a description of the ptrace() system call yet, so
you can't run an inferior process and continue executing gdb under
valgrind, but this should not be difficult to fix.

Valgrind is not magic pixie dust of bug fixing goodness, but it's
yet another great tool to throw in your toolbox when looking at
problems.

Jason

[-- Attachment #2: pa --]
[-- Type: text/plain, Size: 619 bytes --]

2002-03-26  Jason Molenda  (jason-cl@molenda.com)

	* event-top.c (command_line_handler): Don't check penultimate
	byte in zero-length strings.

Index: event-top.c
===================================================================
RCS file: /cvs/src/src/gdb/event-top.c,v
retrieving revision 1.19
diff -u -p -r1.19 event-top.c
--- event-top.c	2002/01/17 22:15:16	1.19
+++ event-top.c	2002/03/27 07:26:42
@@ -681,7 +681,7 @@ command_line_handler (char *rl)
 
   xfree (rl);			/* Allocated in readline.  */
 
-  if (*(p - 1) == '\\')
+  if (p1 != rl && *(p - 1) == '\\')
     {
       p--;			/* Put on top of '\'.  */
 

Re: Minor off-by-one error in command_line_handler

[Andrew Cagney]

> 2002-03-26  Jason Molenda  (jason-cl@molenda.com)
> 
> 	* event-top.c (command_line_handler): Don't check penultimate
> 	byte in zero-length strings.
> 
> 
Yes, and thanks.

Andrew

Re: Minor off-by-one error in command_line_handler

[Andrew Cagney]

> The other problem is with the ALL_BLOCK_SYMBOLS macro.  It looks
> like this
> 
> /* Macro to loop through all symbols in a block BL.
>    i counts which symbol we are looking at, and sym points to the current
>    symbol.  */
> #define ALL_BLOCK_SYMBOLS(bl, i, sym)                   \
>         for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));    \
>              (i) < BLOCK_NSYMS ((bl));                  \
>              ++(i), (sym) = BLOCK_SYM ((bl), (i)))
> 
> Where the block structure (BL) ends with an array of pointers to
> symbols.  The third expression in the for statement increments the
> index variable and reads the address at the i'th element of the
> bl->sym[] array.
> 
> So when a block has 2 symbols, bl->sym[0] and bl->sym[1] contain
> values.  On the last evaluation of this loop, i is pre-incremented
> from 1 to 2 and the statement 'sym = bl->nsym[2]' is done - we're
> reading one element past the end of the array.
> 
> The invalid memory we just read is not used -- the conditional
> expression is then evaluated and the loop exits.  The only way
> I can see this causing a problem is on a system where reading
> that unallocated word of memory would cause a segfault.  Unless
> other people have heard complaints about gdb 5.1 doing so, I
> don't think this is worth worrying about. 

Yes I'd agree. Perhaphs create a very non-critical bug report for this one.

Andrew

Re: Minor off-by-one error in command_line_handler

[Elena Zannoni]

Andrew Cagney writes:
 > > 2002-03-26  Jason Molenda  (jason-cl@molenda.com)
 > > 
 > > 	* event-top.c (command_line_handler): Don't check penultimate
 > > 	byte in zero-length strings.
 > > 
 > > 
 > Yes, and thanks.
 > 
 > Andrew
 > 
 > 

Wait.  This is fine, but, as the comments indicate, the code was taken
from the function command_line_input in top.c, which has the same bug:

      p1 = rl;
      /* Copy line.  Don't copy null at end.  (Leaves line alone
         if this was just a newline)  */
      while (*p1)
	*p++ = *p1++;

      xfree (rl);		/* Allocated in readline.  */

      if (p == linebuffer || *(p - 1) != '\\')
	break;


Jason, can you sneak in a fix for that too?

Thanks
Elena

Re: Minor off-by-one error in command_line_handler

[Andreas Schwab]

Elena Zannoni <ezannoni@redhat.com> writes:

|> Andrew Cagney writes:
|>  > > 2002-03-26  Jason Molenda  (jason-cl@molenda.com)
|>  > > 
|>  > > 	* event-top.c (command_line_handler): Don't check penultimate
|>  > > 	byte in zero-length strings.
|>  > > 
|>  > > 
|>  > Yes, and thanks.
|>  > 
|>  > Andrew
|>  > 
|>  > 
|> 
|> Wait.  This is fine, but, as the comments indicate, the code was taken
|> from the function command_line_input in top.c, which has the same bug:
|> 
|>       p1 = rl;
|>       /* Copy line.  Don't copy null at end.  (Leaves line alone
|>          if this was just a newline)  */
|>       while (*p1)
|> 	*p++ = *p1++;
|> 
|>       xfree (rl);		/* Allocated in readline.  */
|> 
|>       if (p == linebuffer || *(p - 1) != '\\')
|> 	break;

Why?  This one looks ok.

Btw., command_line_handler has this:


  if (*(p - 1) == '\\')
    {
      p--;			/* Put on top of '\'.  */

      if (*p == '\\')

The condition in the last line is always true.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Minor off-by-one error in command_line_handler

[Andreas Schwab]

Elena Zannoni <ezannoni@redhat.com> writes:

|> Andreas Schwab writes:
|>  > 
|>  > Btw., command_line_handler has this:
|>  > 
|>  > 
|>  >   if (*(p - 1) == '\\')
|>  >     {
|>  >       p--;			/* Put on top of '\'.  */
|>  > 
|>  >       if (*p == '\\')
|>  > 
|>  > The condition in the last line is always true.
|>  > 
|> 
|> Yes, thanks for catching this. Would you like to submit a fix?

I've checked this in as being obvious.

2002-03-27  Andreas Schwab  <schwab@suse.de>

	* event-top.c (command_line_handler): Remove useless if.

--- gdb/event-top.c.~1.19.~	2002-01-18 09:37:57.000000000 +0100
+++ gdb/event-top.c	2002-03-27 22:12:23.000000000 +0100
@@ -685,20 +685,17 @@
     {
       p--;			/* Put on top of '\'.  */
 
-      if (*p == '\\')
-	{
-	  readline_input_state.linebuffer = savestring (linebuffer,
-							strlen (linebuffer));
-	  readline_input_state.linebuffer_ptr = p;
-
-	  /* We will not invoke a execute_command if there is more
-	     input expected to complete the command. So, we need to
-	     print an empty prompt here. */
-	  more_to_come = 1;
-	  push_prompt ("", "", "");
-	  display_gdb_prompt (0);
-	  return;
-	}
+      readline_input_state.linebuffer = savestring (linebuffer,
+						    strlen (linebuffer));
+      readline_input_state.linebuffer_ptr = p;
+
+      /* We will not invoke a execute_command if there is more
+	 input expected to complete the command. So, we need to
+	 print an empty prompt here. */
+      more_to_come = 1;
+      push_prompt ("", "", "");
+      display_gdb_prompt (0);
+      return;
     }
 
 #ifdef STOP_SIGNAL

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Minor off-by-one error in command_line_handler

[Elena Zannoni]

Andreas Schwab writes:
 > Elena Zannoni <ezannoni@redhat.com> writes:
 > 
 > |> Andrew Cagney writes:
 > |>  > > 2002-03-26  Jason Molenda  (jason-cl@molenda.com)
 > |>  > > 
 > |>  > > 	* event-top.c (command_line_handler): Don't check penultimate
 > |>  > > 	byte in zero-length strings.
 > |>  > > 
 > |>  > > 
 > |>  > Yes, and thanks.
 > |>  > 
 > |>  > Andrew
 > |>  > 
 > |>  > 
 > |> 
 > |> Wait.  This is fine, but, as the comments indicate, the code was taken
 > |> from the function command_line_input in top.c, which has the same bug:
 > |> 
 > |>       p1 = rl;
 > |>       /* Copy line.  Don't copy null at end.  (Leaves line alone
 > |>          if this was just a newline)  */
 > |>       while (*p1)
 > |> 	*p++ = *p1++;
 > |> 
 > |>       xfree (rl);		/* Allocated in readline.  */
 > |> 
 > |>       if (p == linebuffer || *(p - 1) != '\\')
 > |> 	break;
 > 
 > Why?  This one looks ok.

Oh, You mean because the second part of the 'or' won't be evaluated if 
p==linebuffer (which implies that the line was empty)?

Wonder if this gives an error if run under valgrind (use the --noasync
switch). 

Looking at the logs, the error in event-top.c came in because that
first part of the OR was deleted.

 > 
 > Btw., command_line_handler has this:
 > 
 > 
 >   if (*(p - 1) == '\\')
 >     {
 >       p--;			/* Put on top of '\'.  */
 > 
 >       if (*p == '\\')
 > 
 > The condition in the last line is always true.
 > 

Yes, thanks for catching this. Would you like to submit a fix?

Elena


 > Andreas.
 > 
 > -- 
 > Andreas Schwab, SuSE Labs, schwab@suse.de
 > SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
 > Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
 > "And now for something completely different."

Re: Minor off-by-one error in command_line_handler

[Daniel Jacobowitz]

On Wed, Mar 27, 2002 at 12:01:07AM -0800, Jason Molenda wrote:
> The other problem is with the ALL_BLOCK_SYMBOLS macro.  It looks
> like this
> 
> /* Macro to loop through all symbols in a block BL.
>    i counts which symbol we are looking at, and sym points to the current
>    symbol.  */
> #define ALL_BLOCK_SYMBOLS(bl, i, sym)                   \
>         for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));    \
>              (i) < BLOCK_NSYMS ((bl));                  \
>              ++(i), (sym) = BLOCK_SYM ((bl), (i)))
> 
> Where the block structure (BL) ends with an array of pointers to
> symbols.  The third expression in the for statement increments the
> index variable and reads the address at the i'th element of the
> bl->sym[] array.
> 
> So when a block has 2 symbols, bl->sym[0] and bl->sym[1] contain
> values.  On the last evaluation of this loop, i is pre-incremented
> from 1 to 2 and the statement 'sym = bl->nsym[2]' is done - we're
> reading one element past the end of the array.
> 
> The invalid memory we just read is not used -- the conditional
> expression is then evaluated and the loop exits.  The only way
> I can see this causing a problem is on a system where reading
> that unallocated word of memory would cause a segfault.  Unless
> other people have heard complaints about gdb 5.1 doing so, I
> don't think this is worth worrying about. 

My fault (and after 5.1, I think).  This will cause errors with any
good memory checker, so I suppose it should be fixed.  This incurs a
little slowdown, but was the best way I could think of to do it... OK
to check in?


-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2002-03-29  Daniel Jacobowitz  <drow@mvista.com>

	* symtab.h (ALL_BLOCK_SYMBOLS): Don't dereference the pointer
	after the last symbol in a block.

Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.29
diff -u -p -r1.29 symtab.h
--- symtab.h	2002/03/27 23:10:24	1.29
+++ symtab.h	2002/03/29 07:52:56
@@ -411,7 +411,9 @@ struct block
 #define ALL_BLOCK_SYMBOLS(bl, i, sym)			\
 	for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));	\
 	     (i) < BLOCK_NSYMS ((bl));			\
-	     ++(i), (sym) = BLOCK_SYM ((bl), (i)))
+	     ++(i), (sym) = ((i) < BLOCK_NSYMS ((bl)))	\
+			    ? BLOCK_SYM ((bl), (i))	\
+			    : NULL)
 
 /* Nonzero if symbols of block BL should be sorted alphabetically.
    Don't sort a block which corresponds to a function.  If we did the

Re: Minor off-by-one error in command_line_handler

[Andrew Cagney]

It at least deserves a comment :-)

>  #define ALL_BLOCK_SYMBOLS(bl, i, sym)			\
>  	for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));	\
>  	     (i) < BLOCK_NSYMS ((bl));			\
> -	     ++(i), (sym) = BLOCK_SYM ((bl), (i)))
> +	     ++(i), (sym) = ((i) < BLOCK_NSYMS ((bl)))	\
> +			    ? BLOCK_SYM ((bl), (i))	\
> +			    : NULL)
>  

Andrew

Re: Minor off-by-one error in command_line_handler

[Daniel Jacobowitz]

Does
/* We're careful not to read beyond the last BLOCK_SYM.  */
look about right to you?

On Fri, Mar 29, 2002 at 12:35:33PM -0500, Andrew Cagney wrote:
> It at least deserves a comment :-)
> 
> > #define ALL_BLOCK_SYMBOLS(bl, i, sym)			\
> > 	for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));	\
> > 	     (i) < BLOCK_NSYMS ((bl));			\
> >-	     ++(i), (sym) = BLOCK_SYM ((bl), (i)))
> >+	     ++(i), (sym) = ((i) < BLOCK_NSYMS ((bl)))	\
> >+			    ? BLOCK_SYM ((bl), (i))	\
> >+			    : NULL)
> > 
> 
> Andrew
> 
> 

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: Minor off-by-one error in command_line_handler

[Andrew Cagney]

> Does
> /* We're careful not to read beyond the last BLOCK_SYM.  */
> look about right to you?

Yes, or:

``The hoops are to avoid ... '' :-)

Andrew


> On Fri, Mar 29, 2002 at 12:35:33PM -0500, Andrew Cagney wrote:
> 
>> It at least deserves a comment :-)
>> 
> 
>> > #define ALL_BLOCK_SYMBOLS(bl, i, sym)			\
>> > for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));	\
>> > (i) < BLOCK_NSYMS ((bl));			\
>> >-	     ++(i), (sym) = BLOCK_SYM ((bl), (i)))
>> >+	     ++(i), (sym) = ((i) < BLOCK_NSYMS ((bl)))	\
>> >+			    ? BLOCK_SYM ((bl), (i))	\
>> >+			    : NULL)
>> > 
> 
>> 
>> Andrew
>> 
>> 
> 
> 

Re: Minor off-by-one error in command_line_handler

[Daniel Jacobowitz]

Committed with a comment, then :-)

On Fri, Mar 29, 2002 at 12:35:33PM -0500, Andrew Cagney wrote:
> It at least deserves a comment :-)
> 
> > #define ALL_BLOCK_SYMBOLS(bl, i, sym)			\
> > 	for ((i) = 0, (sym) = BLOCK_SYM ((bl), (i));	\
> > 	     (i) < BLOCK_NSYMS ((bl));			\
> >-	     ++(i), (sym) = BLOCK_SYM ((bl), (i)))
> >+	     ++(i), (sym) = ((i) < BLOCK_NSYMS ((bl)))	\
> >+			    ? BLOCK_SYM ((bl), (i))	\
> >+			    : NULL)
> > 
> 
> Andrew
> 
> 

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer