Minor off-by-one error in command_line_handler

[Jason Molenda <jason-swarelist@molenda.com>]

[-- 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 '\'.  */