From 1052fb3ecb1ae46bcf22634c48739c12e585196a Mon Sep 17 00:00:00 2001 From: Alan Modra Date: Tue, 11 Jul 2023 08:19:20 +0930 Subject: [PATCH] Re: Keeping track of rs6000-coff archive element pointers 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. --- bfd/coff-rs6000.c | 55 ++++++++++++++++++++++++++++++--------------- binutils/bfdtest1.c | 8 ++++++- 2 files changed, 44 insertions(+), 19 deletions(-) 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 "); @@ -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"); -- 2.30.2