pe_ILF_object_p and bfd_check_format_matches

pe_ILF_object_p and bfd_check_format_matches

[Alan Modra]

If pe_ILF_object_p succeeds, pe_ILF_build_a_bfd will have changed the
bfd from being file backed to in-memory.  This can have unfortunate
results for targets checked by bfd_check_format_matches after that
point as they will be matching against the created in-memory image
rather than the file.  bfd_preserve_restore also has a problem if it
flips the BFD_IN_MEMORY flag, because the flag affects iostream
meaning and should be set if using _bfd_memory_iovec.  To fix these
problems, save and restore iostream and iovec along with flags, and
modify bfd_reinit to make the bfd file backed again.  Restoring the
iovec and iostream allows the hack in bfd_reinit keeping BFD_IN_MEMORY
(part of BFD_FLAGS_SAVED) to be removed.
One more detail: If restoring from file backed to in-memory then the
bfd needs to be forcibly removed from the cache lru list, since after
the bfd becomes in-memory a bfd_close will delete the bfd's memory
leaving the lru list pointing into freed memory.

	* cache.c (bfd_cache_init): Clear BFD_CLOSED_BY_CACHE here..
	(bfd_cache_lookup_worker): ..rather than here.
	(bfd_cache_close): Comment.
	* format.c (struct bfd_preserve): Add iovec and iostream fields.
	(bfd_preserve_save): Save them..
	(bfd_preserve_restore): ..and restore them, calling
	bfd_cache_close if the iovec differs.
	(bfd_reinit): Add preserve param.  If the bfd has been flipped
	to in-memory, reopen the file.  Restore flags.
	* peicode.h (pe_ILF_cleanup): New function.
	(pe_ILF_object_p): Return it.
	* bfd.c (BFD_FLAGS_SAVED): Delete.
	* bfd-in2.h: Regenerate.

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index b60ff960f08..a04e97eda67 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -6622,12 +6622,6 @@ struct bfd
   /* Compress sections in this BFD with SHF_COMPRESSED zstd.  */
 #define BFD_COMPRESS_ZSTD      0x400000
 
-  /* Flags bits to be saved in bfd_preserve_save.  */
-#define BFD_FLAGS_SAVED \
-  (BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
-   | BFD_PLUGIN | BFD_COMPRESS_GABI | BFD_CONVERT_ELF_COMMON \
-   | BFD_USE_ELF_STT_COMMON | BFD_COMPRESS_ZSTD)
-
   /* Flags bits which are for BFD use only.  */
 #define BFD_FLAGS_FOR_BFD_USE_MASK \
   (BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 3624bfbc9a5..650df1c79ed 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -181,12 +181,6 @@ CODE_FRAGMENT
 .  {* Compress sections in this BFD with SHF_COMPRESSED zstd.  *}
 .#define BFD_COMPRESS_ZSTD      0x400000
 .
-.  {* Flags bits to be saved in bfd_preserve_save.  *}
-.#define BFD_FLAGS_SAVED \
-.  (BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
-.   | BFD_PLUGIN | BFD_COMPRESS_GABI | BFD_CONVERT_ELF_COMMON \
-.   | BFD_USE_ELF_STT_COMMON | BFD_COMPRESS_ZSTD)
-.
 .  {* Flags bits which are for BFD use only.  *}
 .#define BFD_FLAGS_FOR_BFD_USE_MASK \
 .  (BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
diff --git a/bfd/cache.c b/bfd/cache.c
index ab36c8506bd..3b91cce2307 100644
--- a/bfd/cache.c
+++ b/bfd/cache.c
@@ -266,10 +266,7 @@ bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag)
 	   && !(flag & CACHE_NO_SEEK_ERROR))
     bfd_set_error (bfd_error_system_call);
   else
-    {
-      abfd->flags &= ~BFD_CLOSED_BY_CACHE;
-      return (FILE *) abfd->iostream;
-    }
+    return (FILE *) abfd->iostream;
 
   /* xgettext:c-format */
   _bfd_error_handler (_("reopening %pB: %s"),
@@ -506,6 +503,7 @@ bfd_cache_init (bfd *abfd)
     }
   abfd->iovec = &cache_iovec;
   insert (abfd);
+  abfd->flags &= ~BFD_CLOSED_BY_CACHE;
   ++open_files;
   return true;
 }
@@ -528,6 +526,7 @@ DESCRIPTION
 bool
 bfd_cache_close (bfd *abfd)
 {
+  /* Don't remove this test.  bfd_reinit depends on it.  */
   if (abfd->iovec != &cache_iovec)
     return true;
 
diff --git a/bfd/format.c b/bfd/format.c
index 5ad4190d5c4..dd50b5e653a 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -99,6 +99,8 @@ struct bfd_preserve
   void *marker;
   void *tdata;
   flagword flags;
+  const struct bfd_iovec *iovec;
+  void *iostream;
   const struct bfd_arch_info *arch_info;
   struct bfd_section *sections;
   struct bfd_section *section_last;
@@ -125,6 +127,8 @@ bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve,
   preserve->tdata = abfd->tdata.any;
   preserve->arch_info = abfd->arch_info;
   preserve->flags = abfd->flags;
+  preserve->iovec = abfd->iovec;
+  preserve->iostream = abfd->iostream;
   preserve->sections = abfd->sections;
   preserve->section_last = abfd->section_last;
   preserve->section_count = abfd->section_count;
@@ -143,14 +147,24 @@ bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve,
 /* Clear out a subset of BFD state.  */
 
 static void
-bfd_reinit (bfd *abfd, unsigned int section_id, bfd_cleanup cleanup)
+bfd_reinit (bfd *abfd, unsigned int section_id,
+	    struct bfd_preserve *preserve, bfd_cleanup cleanup)
 {
   _bfd_section_id = section_id;
   if (cleanup)
     cleanup (abfd);
   abfd->tdata.any = NULL;
   abfd->arch_info = &bfd_default_arch_struct;
-  abfd->flags &= BFD_FLAGS_SAVED;
+  if ((abfd->flags & BFD_CLOSED_BY_CACHE) != 0
+      && (abfd->flags & BFD_IN_MEMORY) != 0
+      && (preserve->flags & BFD_CLOSED_BY_CACHE) == 0
+      && (preserve->flags & BFD_IN_MEMORY) == 0)
+    {
+      /* This is to reverse pe_ILF_build_a_bfd, which closes the file
+	 and sets up a bfd in memory.  */
+      bfd_open_file (abfd);
+    }
+  abfd->flags = preserve->flags;
   abfd->build_id = NULL;
   bfd_section_list_clear (abfd);
 }
@@ -164,7 +178,11 @@ bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
 
   abfd->tdata.any = preserve->tdata;
   abfd->arch_info = preserve->arch_info;
+  if (abfd->iovec != preserve->iovec)
+    bfd_cache_close (abfd);
   abfd->flags = preserve->flags;
+  abfd->iovec = preserve->iovec;
+  abfd->iostream = preserve->iostream;
   abfd->section_htab = preserve->section_htab;
   abfd->sections = preserve->sections;
   abfd->section_last = preserve->section_last;
@@ -368,7 +386,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
       /* If we already tried a match, the bfd is modified and may
 	 have sections attached, which will confuse the next
 	 _bfd_check_format call.  */
-      bfd_reinit (abfd, initial_section_id, cleanup);
+      bfd_reinit (abfd, initial_section_id, &preserve, cleanup);
       /* Free bfd_alloc memory too.  If we have matched and preserved
 	 a target then the high water mark is that much higher.  */
       if (preserve_match.marker)
@@ -527,7 +545,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 	 RIGHT_TARG again.  */
       if (match_targ != right_targ)
 	{
-	  bfd_reinit (abfd, initial_section_id, cleanup);
+	  bfd_reinit (abfd, initial_section_id, &preserve, cleanup);
 	  bfd_release (abfd, preserve.marker);
 	  if (bfd_seek (abfd, (file_ptr) 0, SEEK_SET) != 0)
 	    goto err_ret;
diff --git a/bfd/peicode.h b/bfd/peicode.h
index f16aeca7a1b..e2e2be65b5d 100644
--- a/bfd/peicode.h
+++ b/bfd/peicode.h
@@ -1158,6 +1158,17 @@ pe_ILF_build_a_bfd (bfd *	    abfd,
   return false;
 }
 
+/* Cleanup function, returned from check_format hook.  */
+
+static void
+pe_ILF_cleanup (bfd *abfd)
+{
+  struct bfd_in_memory *bim = abfd->iostream;
+  free (bim->buffer);
+  free (bim);
+  abfd->iostream = NULL;
+}
+
 /* We have detected an Import Library Format archive element.
    Decode the element and return the appropriate target.  */
 
@@ -1331,7 +1342,7 @@ pe_ILF_object_p (bfd * abfd)
       return NULL;
     }
 
-  return _bfd_no_cleanup;
+  return pe_ILF_cleanup;
 }
 
 static void

-- 
Alan Modra
Australia Development Lab, IBM

Re: pe_ILF_object_p and bfd_check_format_matches

[Alan Modra]

The last patch wasn't quite correct.  bfd_preserve_restore also needs
to handle an in-memory to file backed transition, seen in a testcase
ILF object matching both pei-arm-little and pei-arm-wince-little.
There the first match is saved in preserve_match, and restored at the
end of the bfd_check_format_matches loop making the bfd in-memory.  On
finding more than one match the function wants to restore the bfd back
to its original state with another bfd_preserve_restore call before
exiting with a bfd_error_file_ambiguously_recognized error.

It is also not correct to restore abfd->iostream unless the iovec
changes.  abfd->iostream is a FILE* when using cache_iovec, and if
the file has been closed and reopened the iostream may have changed.

	* format.c (io_reinit): New function.
	(bfd_reinit, bfd_preserve_restore): Use it.

diff --git a/bfd/format.c b/bfd/format.c
index dd50b5e653a..66b45ae1979 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -144,6 +144,33 @@ bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve,
 			      sizeof (struct section_hash_entry));
 }
 
+/* A back-end object_p function may flip a bfd from file backed to
+   in-memory, eg. pe_ILF_object_p.  In that case to restore the
+   original IO state we need to reopen the file.  Conversely, if we
+   are restoring a previously matched pe ILF format and have been
+   checking further target matches using file IO then we need to close
+   the file and detach the bfd from the cache lru list.  */
+
+static void
+io_reinit (bfd *abfd, struct bfd_preserve *preserve)
+{
+  if (abfd->iovec != preserve->iovec)
+    {
+      /* Handle file backed to in-memory transition.  bfd_cache_close
+	 won't do anything unless abfd->iovec is the cache_iovec.  */
+      bfd_cache_close (abfd);
+      abfd->iovec = preserve->iovec;
+      abfd->iostream = preserve->iostream;
+      /* Handle in-memory to file backed transition.  */
+      if ((abfd->flags & BFD_CLOSED_BY_CACHE) != 0
+	  && (abfd->flags & BFD_IN_MEMORY) != 0
+	  && (preserve->flags & BFD_CLOSED_BY_CACHE) == 0
+	  && (preserve->flags & BFD_IN_MEMORY) == 0)
+	bfd_open_file (abfd);
+    }
+  abfd->flags = preserve->flags;
+}
+
 /* Clear out a subset of BFD state.  */
 
 static void
@@ -155,16 +182,7 @@ bfd_reinit (bfd *abfd, unsigned int section_id,
     cleanup (abfd);
   abfd->tdata.any = NULL;
   abfd->arch_info = &bfd_default_arch_struct;
-  if ((abfd->flags & BFD_CLOSED_BY_CACHE) != 0
-      && (abfd->flags & BFD_IN_MEMORY) != 0
-      && (preserve->flags & BFD_CLOSED_BY_CACHE) == 0
-      && (preserve->flags & BFD_IN_MEMORY) == 0)
-    {
-      /* This is to reverse pe_ILF_build_a_bfd, which closes the file
-	 and sets up a bfd in memory.  */
-      bfd_open_file (abfd);
-    }
-  abfd->flags = preserve->flags;
+  io_reinit (abfd, preserve);
   abfd->build_id = NULL;
   bfd_section_list_clear (abfd);
 }
@@ -178,11 +196,7 @@ bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
 
   abfd->tdata.any = preserve->tdata;
   abfd->arch_info = preserve->arch_info;
-  if (abfd->iovec != preserve->iovec)
-    bfd_cache_close (abfd);
-  abfd->flags = preserve->flags;
-  abfd->iovec = preserve->iovec;
-  abfd->iostream = preserve->iostream;
+  io_reinit (abfd, preserve);
   abfd->section_htab = preserve->section_htab;
   abfd->sections = preserve->sections;
   abfd->section_last = preserve->section_last;

-- 
Alan Modra
Australia Development Lab, IBM

Re: pe_ILF_object_p and bfd_check_format_matches

[Martin Liška]

On 4/13/23 04:58, Alan Modra via Binutils wrote:
> |The last patch wasn't quite correct.|

Heh, I've just noticed that this change caused a crash when ar is called for ~150 files:

==27734== Invalid read of size 4
==27734==    at 0x4D576DD: ftello (in /usr/lib64/libc.so.6)
==27734==    by 0x4958E18: close_one (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x495F2F4: bfd_open_file (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x4B86557: bfd_plugin_open_input (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x4B8674F: try_load_plugin (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x4B86A5B: bfd_plugin_object_p.lto_priv.0 (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x49626EA: bfd_check_format_matches (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x4963016: _bfd_write_archive_contents (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x4968DD5: bfd_close (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734==    by 0x10E9BB: write_archive (in /usr/bin/ar)
==27734==    by 0x1103DD: main (in /usr/bin/ar)
==27734==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Martin

Re: pe_ILF_object_p and bfd_check_format_matches

[Alan Modra]

On Thu, Apr 13, 2023 at 11:55:10AM +0200, Martin Liška wrote:
> On 4/13/23 04:58, Alan Modra via Binutils wrote:
> > |The last patch wasn't quite correct.|
> 
> Heh, I've just noticed that this change caused a crash when ar is called for ~150 files:
> 
> ==27734== Invalid read of size 4
> ==27734==    at 0x4D576DD: ftello (in /usr/lib64/libc.so.6)
> ==27734==    by 0x4958E18: close_one (in /usr/lib64/libbfd-2.40.50.20230412-797.so)

Yes, that will be this part:
> >    It is also not correct to restore abfd->iostream unless the iovec
> >    changes.  abfd->iostream is a FILE* when using cache_iovec, and if
> >    the file has been closed and reopened the iostream may have changed.

I saw a similar invalid read in fseeko, and a double free in fclose
due to the above.

-- 
Alan Modra
Australia Development Lab, IBM

Re: pe_ILF_object_p and bfd_check_format_matches

[Martin Liška]

On 4/13/23 12:58, Alan Modra wrote:
> On Thu, Apr 13, 2023 at 11:55:10AM +0200, Martin Liška wrote:
>> On 4/13/23 04:58, Alan Modra via Binutils wrote:
>>> |The last patch wasn't quite correct.|
>>
>> Heh, I've just noticed that this change caused a crash when ar is called for ~150 files:
>>
>> ==27734== Invalid read of size 4
>> ==27734==    at 0x4D576DD: ftello (in /usr/lib64/libc.so.6)
>> ==27734==    by 0x4958E18: close_one (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
> 
> Yes, that will be this part:
>>>    It is also not correct to restore abfd->iostream unless the iovec
>>>    changes.  abfd->iostream is a FILE* when using cache_iovec, and if
>>>    the file has been closed and reopened the iostream may have changed.
> 
> I saw a similar invalid read in fseeko, and a double free in fclose
> due to the above.
> 

Good, I can confirm the problem is gone with your latest change.

Thanks,
Martin