[RFA] Fix a crash in coffread.c (Was: GDB 6.1 branch 2004-02-26-gmt)

[RFA] Fix a crash in coffread.c (Was: GDB 6.1 branch 2004-02-26-gmt)

[Eli Zaretskii]

> Date: Sat, 21 Feb 2004 18:14:41 +0200
> From: "Eli Zaretskii" <eliz@elta.co.il>
> 
> I built today the latest snapshot of CVS HEAD and found a new
> regression: the DJGPP port crashes at startup while debugging itself.

Bug squashed, I think.  It turned out to be a very old one, actually;
the current CVS HEAD didn't introduce it, it just exposed it because
the GDB binary is now so large.

The bug happens only when GDB (or any other large program) is compiled
with COFF debug info and the line table overflows the 64K limit
allowed by COFF debug info.  I think the patch below fixes that.

Okay to commit?

2004-02-22  Eli Zaretskii  <eliz@elta.co.il>

	* coffread.c (enter_linenos): Don't let rawptr reference memory
	outside linetab[]'s limits.


--- gdb/coffread.c~0	2004-02-14 17:46:32.000000000 +0200
+++ gdb/coffread.c	2004-02-22 22:42:34.000000000 +0200
@@ -1362,11 +1362,15 @@ enter_linenos (long file_offset, int fir
   /* line numbers start at one for the first line of the function */
   first_line--;
 
-  for (;;)
+  /* If the line number is full (e.g. 64K lines in COFF debug info),
+     the next function's L_LNNO32 might not be zero, so don't overstep
+     the table's end in any case.  */
+  for ( ; rawptr <= &linetab[0] + linetab_size; )
     {
       bfd_coff_swap_lineno_in (symfile_bfd, rawptr, &lptr);
       rawptr += local_linesz;
-      /* The next function, or the sentinel, will have L_LNNO32 zero; we exit. */
+      /* The next function, or the sentinel, will have L_LNNO32 zero;
+	 we exit. */
       if (L_LNNO32 (&lptr) && L_LNNO32 (&lptr) <= last_line)
 	record_line (current_subfile, first_line + L_LNNO32 (&lptr),
 		     lptr.l_addr.l_paddr

Re: [RFA] Fix a crash in coffread.c (Was: GDB 6.1 branch 2004-02-26-gmt)

[Elena Zannoni]

Eli Zaretskii writes:
 > > Date: Sat, 21 Feb 2004 18:14:41 +0200
 > > From: "Eli Zaretskii" <eliz@elta.co.il>
 > > 
 > > I built today the latest snapshot of CVS HEAD and found a new
 > > regression: the DJGPP port crashes at startup while debugging itself.
 > 
 > Bug squashed, I think.  It turned out to be a very old one, actually;
 > the current CVS HEAD didn't introduce it, it just exposed it because
 > the GDB binary is now so large.
 > 
 > The bug happens only when GDB (or any other large program) is compiled
 > with COFF debug info and the line table overflows the 64K limit
 > allowed by COFF debug info.  I think the patch below fixes that.
 > 
 > Okay to commit?
 > 
 > 2004-02-22  Eli Zaretskii  <eliz@elta.co.il>
 > 
 > 	* coffread.c (enter_linenos): Don't let rawptr reference memory
 > 	outside linetab[]'s limits.
 > 
 > 
 > --- gdb/coffread.c~0	2004-02-14 17:46:32.000000000 +0200
 > +++ gdb/coffread.c	2004-02-22 22:42:34.000000000 +0200
 > @@ -1362,11 +1362,15 @@ enter_linenos (long file_offset, int fir
 >    /* line numbers start at one for the first line of the function */
 >    first_line--;
 >  
 > -  for (;;)
 > +  /* If the line number is full (e.g. 64K lines in COFF debug info),
                    ^^^^^^^^
                     table?

 > +     the next function's L_LNNO32 might not be zero, so don't overstep
 > +     the table's end in any case.  */
 > +  for ( ; rawptr <= &linetab[0] + linetab_size; )

how about a while loop?

I am not sure I understand how the two cases differ in the layout of
the debug info. Is the beginning of a function still zero valued? Do
we have a function with >64k lines? If we are running beyond the end
of the table, does this mean that we don't read all the debug info we
have?

elena

Re: [RFA] Fix a crash in coffread.c (Was: GDB 6.1 branch 2004-02-26-gmt)

[Eli Zaretskii]

> From: Elena Zannoni <ezannoni@redhat.com>
> Date: Mon, 23 Feb 2004 10:09:09 -0500
> 
> > +  /* If the line number is full (e.g. 64K lines in COFF debug info),
>                     ^^^^^^^^
>                      table?

Yes, a typo.  Thanks for catching it.

> how about a while loop?

Consider it done.

> I am not sure I understand how the two cases differ in the layout of
> the debug info.

Sorry, I don't understand: what two cases?

> Is the beginning of a function still zero valued?

AFAIU, the code tested for the zero-valued L_LNNO32 (&lptr) too late:
the call to bfd_coff_swap_lineno_in is before the test, and it's that
call that caused GDB to crash, since rawptr ran out of the valid
address space.

> Do we have a function with >64k lines?

No, the entire program totals more than 64k lines.

> If we are running beyond the end of the table, does this mean that
> we don't read all the debug info we have?

We do read all the available info.  GNU ld stops writing the table
when it has more than 64k lines (and prints a warning to that effect).
In the cases I debugged, the line table was allocated for precisely
64k lines, a clear sign that the table overflowed during linking (I
also saw the warning).  Since no more info about line numbers is
available, we don't lose anything.  AFAIK, the rest of the debug info,
i.e. the symbol table, is still being read, we just lose information
about source line to code association for some of the functions.

The reason for running beyond the end of the table is, AFAIU, that the
test to terminate the loop is not good enough to catch the end of the
table in time, at least in the case I debugged.  I don't really
understand how it was supposed to make sure that dereferencing rawptr
in libbfd.c:bfd_getl32 (called from bfd_coff_swap_lineno_in) will not
segfault, without an explicit test of rawptr's value; do you?

Re: [RFA] Fix a crash in coffread.c (Was: GDB 6.1 branch 2004-02-26-gmt)

[Elena Zannoni]

Eli Zaretskii writes:
 > > From: Elena Zannoni <ezannoni@redhat.com>
 > > Date: Mon, 23 Feb 2004 10:09:09 -0500
 > > 
 > > > +  /* If the line number is full (e.g. 64K lines in COFF debug info),
 > >                     ^^^^^^^^
 > >                      table?
 > 
 > Yes, a typo.  Thanks for catching it.
 > 
 > > how about a while loop?
 > 
 > Consider it done.
 > 
 > > I am not sure I understand how the two cases differ in the layout of
 > > the debug info.
 > 
 > Sorry, I don't understand: what two cases?

with and w/o the max reached. You explain below.

 > 
 > > Is the beginning of a function still zero valued?
 > 
 > AFAIU, the code tested for the zero-valued L_LNNO32 (&lptr) too late:
 > the call to bfd_coff_swap_lineno_in is before the test, and it's that
 > call that caused GDB to crash, since rawptr ran out of the valid
 > address space.
 > 

ah, right.

 > > Do we have a function with >64k lines?
 > 
 > No, the entire program totals more than 64k lines.
 > 

ok, I am not too familiar with the layout. I guess there is a big
table with 0 entries to mark functions.

 > > If we are running beyond the end of the table, does this mean that
 > > we don't read all the debug info we have?
 > 
 > We do read all the available info.  GNU ld stops writing the table
 > when it has more than 64k lines (and prints a warning to that effect).
 > In the cases I debugged, the line table was allocated for precisely
 > 64k lines, a clear sign that the table overflowed during linking (I
 > also saw the warning).  Since no more info about line numbers is
 > available, we don't lose anything.  AFAIK, the rest of the debug info,
 > i.e. the symbol table, is still being read, we just lose information
 > about source line to code association for some of the functions.
 > 

ah, ok. I was wondering at what stage the information was lost. We
don't have to worry about it then.

 > The reason for running beyond the end of the table is, AFAIU, that the
 > test to terminate the loop is not good enough to catch the end of the
 > table in time, at least in the case I debugged.  I don't really
 > understand how it was supposed to make sure that dereferencing rawptr
 > in libbfd.c:bfd_getl32 (called from bfd_coff_swap_lineno_in) will not
 > segfault, without an explicit test of rawptr's value; do you?

No, probably it was one of those "it will never happen" things. I've
seen those assumptions before...

Ok, then, check it in.

elena