Re: Keeping track of rs6000-coff archive element pointers

Re: Keeping track of rs6000-coff archive element pointers

[Alan Modra]

See https://sourceware.org/pipermail/gdb-patches/2023-July/200772.html
thread.  I'm going to apply this to mainline and binutils-2.41.

bfd/
	* coff-rs6000.c (add_range): Revise comment, noting possible fail.
	(_bfd_xcoff_openr_next_archived_file): Start with clean ranges.
binutils/
	* bfdtest1.c: Enhance to catch errors on second scan.

diff --git a/bfd/coff-rs6000.c b/bfd/coff-rs6000.c
index 271a24fff69..a692c1ae474 100644
--- a/bfd/coff-rs6000.c
+++ b/bfd/coff-rs6000.c
@@ -1598,14 +1598,32 @@ _bfd_xcoff_archive_p (bfd *abfd)
 
 /* Track file ranges occupied by elements.  Add [START,END) to the
    list of ranges and return TRUE if there is no overlap between the
-   new and any other element or the archive file header.  Note that
-   this would seem to preclude calling _bfd_get_elt_at_filepos twice
-   for the same element, but we won't get to _bfd_xcoff_read_ar_hdr if
-   an element is read more than once.  See _bfd_get_elt_at_filepos use
-   of _bfd_look_for_bfd_in_cache.  Also, the xcoff archive code
-   doesn't call _bfd_read_ar_hdr when reading the armap, nor does it
-   need to use extended name tables.  So those other routines in
-   archive.c that call _bfd_read_ar_hdr are unused.  */
+   new and any other element or the archive file header.  This is
+   aimed at preventing infinite looping on malformed archives, for
+   "ar" and similar which typically use code like:
+   .  for (last = bfd_openr_next_archived_file (archive, NULL);
+   .       last;
+   .       last = next)
+   .    {
+   .      do_something_with (last);
+   .      next = bfd_openr_next_archived_file (archive, last);
+   .      bfd_close (last);
+   .    }
+   The check implemented here is only possible due to the fact that
+   for XCOFF archives bfd_openr_next_archived_file is the only code
+   path leading to _bfd_read_ar_hdr.  _bfd_read_ar_hdr is not called
+   when reading the armap, nor do XCOFF archives use the extended name
+   scheme implemented in archive.c.
+
+   Note that the check relies on the previous element being closed,
+   and there is one case where add_range might fail but I think it is
+   sufficently unusual that it doesn't warrant fixing:
+   If the loop body above called bfd_openr_next_archived_file twice
+   with the same arguments and the element returned is bfd_close'd
+   between those calls then we'll return false here for the second
+   call.  (For why this is so see _bfd_look_for_bfd_in_cache in
+   _bfd_get_elt_at_filepos, and know that bfd_close removes elements
+   from the cache.)  */
 
 static bool
 add_range (bfd *abfd, ufile_ptr start, ufile_ptr end)
@@ -1770,12 +1788,14 @@ _bfd_xcoff_openr_next_archived_file (bfd *archive, bfd *last_file)
     {
       if (last_file == NULL)
 	{
+	  /* If we are scanning over elements twice in an open archive,
+	     which can happen in gdb after a fork, ensure we start the
+	     second scan with clean ranges.  */
+	  x_artdata (archive)->ranges.start = 0;
+	  x_artdata (archive)->ranges.end = SIZEOF_AR_FILE_HDR;
+	  x_artdata (archive)->ranges.next = NULL;
+	  x_artdata (archive)->ar_hdr_size = SIZEOF_AR_HDR;
 	  filestart = bfd_ardata (archive)->first_file_filepos;
-	  if (x_artdata (archive)->ar_hdr_size == 0)
-	    {
-	      x_artdata (archive)->ranges.end = SIZEOF_AR_FILE_HDR;
-	      x_artdata (archive)->ar_hdr_size = SIZEOF_AR_HDR;
-	    }
 	}
       else
 	GET_VALUE_IN_FIELD (filestart, arch_xhdr (last_file)->nextoff, 10);
@@ -1794,12 +1814,11 @@ _bfd_xcoff_openr_next_archived_file (bfd *archive, bfd *last_file)
     {
       if (last_file == NULL)
 	{
+	  x_artdata (archive)->ranges.start = 0;
+	  x_artdata (archive)->ranges.end = SIZEOF_AR_FILE_HDR_BIG;
+	  x_artdata (archive)->ranges.next = NULL;
+	  x_artdata (archive)->ar_hdr_size = SIZEOF_AR_HDR_BIG;
 	  filestart = bfd_ardata (archive)->first_file_filepos;
-	  if (x_artdata (archive)->ar_hdr_size == 0)
-	    {
-	      x_artdata (archive)->ranges.end = SIZEOF_AR_FILE_HDR_BIG;
-	      x_artdata (archive)->ar_hdr_size = SIZEOF_AR_HDR_BIG;
-	    }
 	}
       else
 	GET_VALUE_IN_FIELD (filestart, arch_xhdr_big (last_file)->nextoff, 10);
diff --git a/binutils/bfdtest1.c b/binutils/bfdtest1.c
index 72355723479..b81f48b13fe 100644
--- a/binutils/bfdtest1.c
+++ b/binutils/bfdtest1.c
@@ -33,6 +33,7 @@ main (int argc, char **argv)
 {
   bfd *archive;
   bfd *last, *next;
+  int count;
 
   if (argc != 2)
     die ("usage: bfdtest1 <archive>");
@@ -47,12 +48,13 @@ main (int argc, char **argv)
       die ("bfd_check_format");
     }
 
-  for (last = bfd_openr_next_archived_file (archive, NULL);
+  for (count = 0, last = bfd_openr_next_archived_file (archive, NULL);
        last;
        last = next)
     {
       next = bfd_openr_next_archived_file (archive, last);
       bfd_close (last);
+      count++;
     }
 
   for (last = bfd_openr_next_archived_file (archive, NULL);
@@ -61,8 +63,12 @@ main (int argc, char **argv)
     {
       next = bfd_openr_next_archived_file (archive, last);
       bfd_close (last);
+      count--;
     }
 
+  if (count != 0)
+    die ("element count differs in second scan");
+
   if (!bfd_close (archive))
     die ("bfd_close");
 

-- 
Alan Modra
Australia Development Lab, IBM