RFC: Fix an infinite loop placing sections in relocatable objects

RFC: Fix an infinite loop placing sections in relocatable objects

[Daniel Jacobowitz]

While trying to track down something completely different, I accidentally
got GDB stuck in an infinite loop.  I'm not entirely sure what I was
thinking when I wrote this, but I believe the attached patch is a correct
fix:
  - if we can't place at this address, we bump start_addr, so we should
    restart the inner for loop.
  - arg->lowest and align are both invariant in the function up to this
    point, so there's no point resetting start_addr inside the loop and
    clobbering the retry logic.

Tested x86_64-pc-linux-gnu, where I can no longer trigger an infinite loop
here.  Any opinions?

-- 
Daniel Jacobowitz
CodeSourcery

2006-02-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* symfile.c (place_section): Correct retry logic.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.167
diff -u -p -r1.167 symfile.c
--- symfile.c	7 Feb 2006 19:40:30 -0000	1.167
+++ symfile.c	16 Feb 2006 15:21:43 -0000
@@ -476,6 +476,7 @@ place_section (bfd *abfd, asection *sect
   struct place_section_arg *arg = obj;
   CORE_ADDR *offsets = arg->offsets->offsets, start_addr;
   int done;
+  ULONGEST align = 1 << bfd_get_section_alignment (abfd, sect);
 
   /* We are only interested in loadable sections.  */
   if ((bfd_get_section_flags (abfd, sect) & SEC_LOAD) == 0)
@@ -486,11 +487,11 @@ place_section (bfd *abfd, asection *sect
     return;
 
   /* Otherwise, let's try to find a place for the section.  */
+  start_addr = (arg->lowest + align - 1) & -align;
+
   do {
     asection *cur_sec;
-    ULONGEST align = 1 << bfd_get_section_alignment (abfd, sect);
 
-    start_addr = (arg->lowest + align - 1) & -align;
     done = 1;
 
     for (cur_sec = abfd->sections; cur_sec != NULL; cur_sec = cur_sec->next)
@@ -524,7 +525,7 @@ place_section (bfd *abfd, asection *sect
 	    start_addr = offsets[indx] + bfd_get_section_size (cur_sec);
 	    start_addr = (start_addr + align - 1) & -align;
 	    done = 0;
-	    continue;
+	    break;
 	  }
 
 	/* Otherwise, we appear to be OK.  So far.  */

Re: RFC: Fix an infinite loop placing sections in relocatable objects

[Jim Blandy]

> Tested x86_64-pc-linux-gnu, where I can no longer trigger an infinite loop
> here.  Any opinions?

I think you're right.  Resetting start_addr at the top of the 'do'
loop's body is clearly broken.

Re: RFC: Fix an infinite loop placing sections in relocatable objects

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> diff -u -p -r1.167 symfile.c
> --- symfile.c	7 Feb 2006 19:40:30 -0000	1.167
> +++ symfile.c	16 Feb 2006 15:21:43 -0000
> @@ -476,6 +476,7 @@ place_section (bfd *abfd, asection *sect
>    struct place_section_arg *arg = obj;
>    CORE_ADDR *offsets = arg->offsets->offsets, start_addr;
>    int done;
> +  ULONGEST align = 1 << bfd_get_section_alignment (abfd, sect);

I think bfd_get_section_alignment can return values bigger than 31 in
general, and this is undefined in this case.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: RFC: Fix an infinite loop placing sections in relocatable objects

[Daniel Jacobowitz]

On Thu, Feb 16, 2006 at 11:04:31PM +0100, Andreas Schwab wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > diff -u -p -r1.167 symfile.c
> > --- symfile.c	7 Feb 2006 19:40:30 -0000	1.167
> > +++ symfile.c	16 Feb 2006 15:21:43 -0000
> > @@ -476,6 +476,7 @@ place_section (bfd *abfd, asection *sect
> >    struct place_section_arg *arg = obj;
> >    CORE_ADDR *offsets = arg->offsets->offsets, start_addr;
> >    int done;
> > +  ULONGEST align = 1 << bfd_get_section_alignment (abfd, sect);
> 
> I think bfd_get_section_alignment can return values bigger than 31 in
> general, and this is undefined in this case.

It's only used in math with a CORE_ADDR, which will be the size of a
bfd_vma.  Can bfd_get_section_alignment return a shift greater than the
size of a bfd_vma?  What would that mean?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Fix an infinite loop placing sections in relocatable objects

[Jim Blandy]

On 2/16/06, Daniel Jacobowitz <drow@false.org> wrote:
> > > +  ULONGEST align = 1 << bfd_get_section_alignment (abfd, sect);
> >
> > I think bfd_get_section_alignment can return values bigger than 31 in
> > general, and this is undefined in this case.
>
> It's only used in math with a CORE_ADDR, which will be the size of a
> bfd_vma.  Can bfd_get_section_alignment return a shift greater than the
> size of a bfd_vma?  What would that mean?

No, what he means is that the literal "1" is just an 'int', so the
shift will be done in the width of an int.  The conversion to ULONGEST
happens later.  The fix is to cast the 1 to ULONGEST before shifting
it.

Re: RFC: Fix an infinite loop placing sections in relocatable objects

[Daniel Jacobowitz]

On Thu, Feb 16, 2006 at 02:23:47PM -0800, Jim Blandy wrote:
> On 2/16/06, Daniel Jacobowitz <drow@false.org> wrote:
> > > > +  ULONGEST align = 1 << bfd_get_section_alignment (abfd, sect);
> > >
> > > I think bfd_get_section_alignment can return values bigger than 31 in
> > > general, and this is undefined in this case.
> >
> > It's only used in math with a CORE_ADDR, which will be the size of a
> > bfd_vma.  Can bfd_get_section_alignment return a shift greater than the
> > size of a bfd_vma?  What would that mean?
> 
> No, what he means is that the literal "1" is just an 'int', so the
> shift will be done in the width of an int.  The conversion to ULONGEST
> happens later.  The fix is to cast the 1 to ULONGEST before shifting
> it.

Oh!  Right.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Fix an infinite loop placing sections in relocatable objects

[Daniel Jacobowitz]

On Thu, Feb 16, 2006 at 10:24:44AM -0500, Daniel Jacobowitz wrote:
> While trying to track down something completely different, I accidentally
> got GDB stuck in an infinite loop.  I'm not entirely sure what I was
> thinking when I wrote this, but I believe the attached patch is a correct
> fix:
>   - if we can't place at this address, we bump start_addr, so we should
>     restart the inner for loop.
>   - arg->lowest and align are both invariant in the function up to this
>     point, so there's no point resetting start_addr inside the loop and
>     clobbering the retry logic.
> 
> Tested x86_64-pc-linux-gnu, where I can no longer trigger an infinite loop
> here.  Any opinions?

I've checked in this version; thanks to both Jim and Andreas.


-- 
Daniel Jacobowitz
CodeSourcery

2006-02-20  Daniel Jacobowitz  <dan@codesourcery.com>

	* symfile.c (place_section): Correct retry logic.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.167
diff -u -p -r1.167 symfile.c
--- symfile.c	7 Feb 2006 19:40:30 -0000	1.167
+++ symfile.c	20 Feb 2006 14:58:31 -0000
@@ -476,6 +476,7 @@ place_section (bfd *abfd, asection *sect
   struct place_section_arg *arg = obj;
   CORE_ADDR *offsets = arg->offsets->offsets, start_addr;
   int done;
+  ULONGEST align = ((ULONGEST) 1) << bfd_get_section_alignment (abfd, sect);
 
   /* We are only interested in loadable sections.  */
   if ((bfd_get_section_flags (abfd, sect) & SEC_LOAD) == 0)
@@ -486,11 +487,11 @@ place_section (bfd *abfd, asection *sect
     return;
 
   /* Otherwise, let's try to find a place for the section.  */
+  start_addr = (arg->lowest + align - 1) & -align;
+
   do {
     asection *cur_sec;
-    ULONGEST align = 1 << bfd_get_section_alignment (abfd, sect);
 
-    start_addr = (arg->lowest + align - 1) & -align;
     done = 1;
 
     for (cur_sec = abfd->sections; cur_sec != NULL; cur_sec = cur_sec->next)
@@ -524,7 +525,7 @@ place_section (bfd *abfd, asection *sect
 	    start_addr = offsets[indx] + bfd_get_section_size (cur_sec);
 	    start_addr = (start_addr + align - 1) & -align;
 	    done = 0;
-	    continue;
+	    break;
 	  }
 
 	/* Otherwise, we appear to be OK.  So far.  */