[binutils-gdb] PR23254, ld.bfd mishandles file pointers while scanning archive

[Alan Modra <amodra at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=27b0767593284f97384b3597ebd211164f8c8b47

commit 27b0767593284f97384b3597ebd211164f8c8b47
Author: Alan Modra <amodra@gmail.com>
Date:   Tue Jun 5 21:04:00 2018 +0930

    PR23254, ld.bfd mishandles file pointers while scanning archive
    
    Best practice is to not mix lseek/read with fseek/fread on the same
    underlying file descriptor, as not all stdio implementations will cope.
    Since the plugin uses lseek/read while bfd uses fseek/fread this patch
    reopens the file for exclusive use by the plugin rather than trying to
    restore the file descriptor.  That allows the plugin to read the file
    after plugin_call_claim_file too.
    
    bfd/
    	PR 23254
    	* plugin.c (bfd_plugin_open_input): Allow for possibility of
    	nested archives.  Open file again for plugin.
    	(try_claim): Don't save and restore file position.  Close file
    	if not claimed.
    	* sysdep.h (O_BINARY): Define.
    ld/
    	PR 23254
    	* plugin.c (plugin_call_claim_file): Revert 2016-07-19 patch.
    	(plugin_object_p): Don't dup file descriptor.

Diff:
---
 bfd/ChangeLog |  9 +++++++++
 bfd/plugin.c  | 20 ++++++++++++++------
 bfd/sysdep.h  |  4 ++++
 ld/ChangeLog  |  6 ++++++
 ld/plugin.c   | 10 ----------
 5 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index d776d0b..3688cf2 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,12 @@
+2018-06-05  Alan Modra  <amodra@gmail.com>
+
+	PR 23254
+	* plugin.c (bfd_plugin_open_input): Allow for possibility of
+	nested archives.  Open file again for plugin.
+	(try_claim): Don't save and restore file position.  Close file
+	if not claimed.
+	* sysdep.h (O_BINARY): Define.
+
 2018-06-04  Max Filippov  <jcmvbkbc@gmail.com>
 
 	* elf32-xtensa.c (xtensa_read_table_entries): Make global.
diff --git a/bfd/plugin.c b/bfd/plugin.c
index 16a706a..7c5bba2 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -166,14 +166,22 @@ bfd_plugin_open_input (bfd *ibfd, struct ld_plugin_input_file *file)
   bfd *iobfd;
 
   iobfd = ibfd;
-  if (ibfd->my_archive && !bfd_is_thin_archive (ibfd->my_archive))
-    iobfd = ibfd->my_archive;
+  while (iobfd->my_archive
+	 && !bfd_is_thin_archive (iobfd->my_archive))
+    iobfd = iobfd->my_archive;
   file->name = iobfd->filename;
 
   if (!iobfd->iostream && !bfd_open_file (iobfd))
     return 0;
 
-  file->fd = fileno ((FILE *) iobfd->iostream);
+  /* The plugin API expects that the file descriptor won't be closed
+     and reused as done by the bfd file cache.  So open it again.
+     dup isn't good enough.  plugin IO uses lseek/read while BFD uses
+     fseek/fread.  It isn't wise to mix the unistd and stdio calls on
+     the same underlying file descriptor.  */
+  file->fd = open (file->name, O_RDONLY | O_BINARY);
+  if (file->fd < 0)
+    return 0;
 
   if (iobfd == ibfd)
     {
@@ -197,12 +205,12 @@ try_claim (bfd *abfd)
   int claimed = 0;
   struct ld_plugin_input_file file;
 
+  file.handle = abfd;
   if (!bfd_plugin_open_input (abfd, &file))
     return 0;
-  file.handle = abfd;
-  off_t cur_offset = lseek (file.fd, 0, SEEK_CUR);
   claim_file (&file, &claimed);
-  lseek (file.fd, cur_offset, SEEK_SET);
+  if (!claimed)
+    close (file.fd);
   return claimed;
 }
 
diff --git a/bfd/sysdep.h b/bfd/sysdep.h
index 840c028..bc7dbcf 100644
--- a/bfd/sysdep.h
+++ b/bfd/sysdep.h
@@ -108,6 +108,10 @@ extern char *strrchr ();
 #ifndef O_ACCMODE
 #define O_ACCMODE (O_RDONLY | O_WRONLY | O_RDWR)
 #endif
+/* Systems that don't already define this, don't need it.  */
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
 
 #ifndef SEEK_SET
 #define SEEK_SET 0
diff --git a/ld/ChangeLog b/ld/ChangeLog
index 7d9b456..9b6d081 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-05  Alan Modra  <amodra@gmail.com>
+
+	PR 23254
+	* plugin.c (plugin_call_claim_file): Revert 2016-07-19 patch.
+	(plugin_object_p): Don't dup file descriptor.
+
 2018-06-05  Flavio Ceolin  <flavio.ceolin@intel.com>
 
 	* testsuite/ld-elf/elf.exp Run new test.
diff --git a/ld/plugin.c b/ld/plugin.c
index fad8bc0..78f2e04 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -1053,14 +1053,10 @@ plugin_call_claim_file (const struct ld_plugin_input_file *file, int *claimed)
     {
       if (curplug->claim_file_handler)
 	{
-	  off_t cur_offset;
 	  enum ld_plugin_status rv;
 
 	  called_plugin = curplug;
-	  cur_offset = lseek (file->fd, 0, SEEK_CUR);
 	  rv = (*curplug->claim_file_handler) (file, claimed);
-	  if (!*claimed)
-	    lseek (file->fd, cur_offset, SEEK_SET);
 	  called_plugin = NULL;
 	  if (rv != LDPS_OK)
 	    set_plugin_error (curplug->name);
@@ -1126,12 +1122,6 @@ plugin_object_p (bfd *ibfd)
     }
 
   file.handle = input;
-  /* The plugin API expects that the file descriptor won't be closed
-     and reused as done by the bfd file cache.  So dup one.  */
-  file.fd = dup (file.fd);
-  if (file.fd < 0)
-    return NULL;
-
   input->abfd = abfd;
   input->view_buffer.addr = NULL;
   input->view_buffer.filesize = 0;