[v2] bfd: Close the file descriptor if there is no archive fd

Message ID CAMe9rOqSSoqY69PYKQ2vJoGjZfuPVa+xyE+RVbKB-qx6TrT-0w@mail.gmail.com
State New
Headers show
Series
  • [v2] bfd: Close the file descriptor if there is no archive fd
Related show

Commit Message

Cooper Qu via Binutils July 27, 2021, 2:28 p.m.
On Mon, Jul 26, 2021 at 6:44 PM Alan Modra <amodra@gmail.com> wrote:
>

> On Mon, Jul 26, 2021 at 06:21:31AM -0700, H.J. Lu via Binutils wrote:

> > Close the thin archive file descriptor since it can't be used directly

> > to access archive members.  This avoids running out of file descriptors

> > on thin archives with many archive members.

> >

> >       PR ld/28138

> >       * plugin.c (bfd_plugin_close_file_descriptor): Close the thin

> >       archive file descriptor.

>

> > +      /* Close the thin archive file descriptor since it can't be used

> > +         directly to access archive members.  */

>

> At first reading, I understood the code better without the comment.

> I think it would be better if you omit this comment and instead put a

> comment before the while loop, if you feel you need a comment.


I changed it to

      /* Close the file descriptor if there is no archive plugin file
         descriptor.  */
      if (abfd->archive_plugin_fd == -1)
        {
          close (fd);
          return;
        }

> Also, please don't pass in a NULL abfd for non-archives.  Instead put

> the my_archive test in bfd_plugin_close_file_descriptor.

>


I prefer to keep it ASIS for

ld/plugin.c:      bfd_plugin_close_file_descriptor (input->ibfd, input->fd);

      ^^^^^^^^^^^ This is NULL.

> I suspect you also need to tweak bfd_plugin_open_input, specifically

>   /* Ruse the archive plugin file descriptor.  */

>   if (iobfd != ibfd)

> might be wrong for nested archives.


It is handled correctly:

 /* Reuse the archive plugin file descriptor.  */
  if (iobfd != ibfd)
    fd = iobfd->archive_plugin_fd;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This isn't -1 only if there is a normal archive.
  else
    fd = -1;

  if (fd < 0)

> --

> Alan Modra

> Australia Development Lab, IBM


Here is the v2 patch.  OK for master?

Thanks.

-- 
H.J.

Comments

Cooper Qu via Binutils July 27, 2021, 7:45 p.m. | #1
On Tue, Jul 27, 2021 at 7:28 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Mon, Jul 26, 2021 at 6:44 PM Alan Modra <amodra@gmail.com> wrote:

> >

> > On Mon, Jul 26, 2021 at 06:21:31AM -0700, H.J. Lu via Binutils wrote:

> > > Close the thin archive file descriptor since it can't be used directly

> > > to access archive members.  This avoids running out of file descriptors

> > > on thin archives with many archive members.

> > >

> > >       PR ld/28138

> > >       * plugin.c (bfd_plugin_close_file_descriptor): Close the thin

> > >       archive file descriptor.

> >

> > > +      /* Close the thin archive file descriptor since it can't be used

> > > +         directly to access archive members.  */

> >

> > At first reading, I understood the code better without the comment.

> > I think it would be better if you omit this comment and instead put a

> > comment before the while loop, if you feel you need a comment.

>

> I changed it to

>

>       /* Close the file descriptor if there is no archive plugin file

>          descriptor.  */

>       if (abfd->archive_plugin_fd == -1)

>         {

>           close (fd);

>           return;

>         }

>

> > Also, please don't pass in a NULL abfd for non-archives.  Instead put

> > the my_archive test in bfd_plugin_close_file_descriptor.

> >

>

> I prefer to keep it ASIS for

>

> ld/plugin.c:      bfd_plugin_close_file_descriptor (input->ibfd, input->fd);

>

>       ^^^^^^^^^^^ This is NULL.

>

> > I suspect you also need to tweak bfd_plugin_open_input, specifically

> >   /* Ruse the archive plugin file descriptor.  */

> >   if (iobfd != ibfd)

> > might be wrong for nested archives.

>

> It is handled correctly:

>

>  /* Reuse the archive plugin file descriptor.  */

>   if (iobfd != ibfd)

>     fd = iobfd->archive_plugin_fd;

> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This isn't -1 only if there is a normal archive.

>   else

>     fd = -1;

>

>   if (fd < 0)

>

> > --

> > Alan Modra

> > Australia Development Lab, IBM

>

> Here is the v2 patch.  OK for master?


Here is the v3 patch with a testcase.


-- 
H.J.
Cooper Qu via Binutils July 28, 2021, 3:11 a.m. | #2
On Tue, Jul 27, 2021 at 12:45:59PM -0700, H.J. Lu wrote:
> Here is the v3 patch with a testcase.


OK, after you have tested this on more than just x86, and fixed the
fails due to trying to run non-native executables.

-- 
Alan Modra
Australia Development Lab, IBM
Cooper Qu via Binutils July 28, 2021, 3:41 a.m. | #3
On Tue, Jul 27, 2021 at 8:11 PM Alan Modra <amodra@gmail.com> wrote:
>

> On Tue, Jul 27, 2021 at 12:45:59PM -0700, H.J. Lu wrote:

> > Here is the v3 patch with a testcase.

>

> OK, after you have tested this on more than just x86, and fixed the

> fails due to trying to run non-native executables.

>


Here is the v4 patch I am checking in.   Tested with s390x-linux cross
compiler.

-- 
H.J.
From af7449d42ade25304b746a43b8ede672baac1d68 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 26 Jul 2021 05:59:55 -0700
Subject: [PATCH v4] bfd: Close the file descriptor if there is no archive fd

Close the file descriptor if there is no archive plugin file descriptor
to avoid running out of file descriptors on thin archives with many
archive members.

bfd/

	PR ld/28138
	* plugin.c (bfd_plugin_close_file_descriptor): Close the file
	descriptor there is no archive plugin file descriptor.

ld/

	PR ld/28138
	* testsuite/ld-plugin/lto.exp: Run ld/28138 tests.
	* testsuite/ld-plugin/pr28138.c: New file.
	* testsuite/ld-plugin/pr28138-1.c: Likewise.
	* testsuite/ld-plugin/pr28138-2.c: Likewise.
	* testsuite/ld-plugin/pr28138-3.c: Likewise.
	* testsuite/ld-plugin/pr28138-4.c: Likewise.
	* testsuite/ld-plugin/pr28138-5.c: Likewise.
	* testsuite/ld-plugin/pr28138-6.c: Likewise.
	* testsuite/ld-plugin/pr28138-7.c: Likewise.
---
 bfd/plugin.c                       |  8 +++++++
 ld/testsuite/ld-plugin/lto.exp     | 34 ++++++++++++++++++++++++++++++
 ld/testsuite/ld-plugin/pr28138-1.c |  6 ++++++
 ld/testsuite/ld-plugin/pr28138-2.c |  6 ++++++
 ld/testsuite/ld-plugin/pr28138-3.c |  6 ++++++
 ld/testsuite/ld-plugin/pr28138-4.c |  6 ++++++
 ld/testsuite/ld-plugin/pr28138-5.c |  6 ++++++
 ld/testsuite/ld-plugin/pr28138-6.c |  6 ++++++
 ld/testsuite/ld-plugin/pr28138-7.c |  6 ++++++
 ld/testsuite/ld-plugin/pr28138.c   | 20 ++++++++++++++++++
 10 files changed, 104 insertions(+)
 create mode 100644 ld/testsuite/ld-plugin/pr28138-1.c
 create mode 100644 ld/testsuite/ld-plugin/pr28138-2.c
 create mode 100644 ld/testsuite/ld-plugin/pr28138-3.c
 create mode 100644 ld/testsuite/ld-plugin/pr28138-4.c
 create mode 100644 ld/testsuite/ld-plugin/pr28138-5.c
 create mode 100644 ld/testsuite/ld-plugin/pr28138-6.c
 create mode 100644 ld/testsuite/ld-plugin/pr28138-7.c
 create mode 100644 ld/testsuite/ld-plugin/pr28138.c

diff --git a/bfd/plugin.c b/bfd/plugin.c
index 6cfa2b66470..3bab8febe88 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -291,6 +291,14 @@ bfd_plugin_close_file_descriptor (bfd *abfd, int fd)
 	     && !bfd_is_thin_archive (abfd->my_archive))
 	abfd = abfd->my_archive;
 
+      /* Close the file descriptor if there is no archive plugin file
+	 descriptor.  */
+      if (abfd->archive_plugin_fd == -1)
+	{
+	  close (fd);
+	  return;
+	}
+
       abfd->archive_plugin_fd_open_count--;
       /* Dup the archive plugin file descriptor for later use, which
 	 will be closed by _bfd_archive_close_and_cleanup.  */
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index def69e43ab3..999d911ce6a 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -687,6 +687,40 @@ if { [is_elf_format] && [check_lto_shared_available] } {
     }
 }
 
+run_cc_link_tests [list \
+    [list \
+	"Build pr28138.a" \
+	"-T" "" \
+	{pr28138-1.c pr28138-2.c pr28138-3.c pr28138-4.c pr28138-5.c \
+	 pr28138-6.c pr28138-7.c} {} "pr28138.a" \
+    ] \
+    [list \
+	"Build pr28138.o" \
+	"" "" \
+	{pr28138.c} {} \
+    ] \
+]
+
+set exec_output [run_host_cmd "sh" \
+			      "-c \"ulimit -n 20; \
+			      $CC -Btmpdir/ld -o tmpdir/pr28138 \
+			      tmpdir/pr28138.o tmpdir/pr28138.a\""]
+set exec_output [prune_warnings $exec_output]
+if [string match "" $exec_output] then {
+    if { [isnative] } {
+	set exec_output [run_host_cmd "tmpdir/pr28138" ""]
+	if [string match "PASS" $exec_output] then {
+	    pass "PR ld/28138"
+	} else {
+	    fail "PR ld/28138"
+	}
+    } else {
+	pass "PR ld/28138"
+    }
+} else {
+    fail "PR ld/28138"
+}
+
 set testname "Build liblto-11.a"
 remote_file host delete "tmpdir/liblto-11.a"
 set catch_output [run_host_cmd "$ar" "rc $plug_opt tmpdir/liblto-11.a tmpdir/lto-11a.o tmpdir/lto-11b.o tmpdir/lto-11c.o"]
diff --git a/ld/testsuite/ld-plugin/pr28138-1.c b/ld/testsuite/ld-plugin/pr28138-1.c
new file mode 100644
index 00000000000..51d119e1642
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138-1.c
@@ -0,0 +1,6 @@
+extern int a0(void);
+int
+a1(void)
+{
+  return 1 + a0();
+}
diff --git a/ld/testsuite/ld-plugin/pr28138-2.c b/ld/testsuite/ld-plugin/pr28138-2.c
new file mode 100644
index 00000000000..1120cd797e9
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138-2.c
@@ -0,0 +1,6 @@
+extern int a1(void);
+int
+a2(void)
+{
+  return 1 + a1();
+}
diff --git a/ld/testsuite/ld-plugin/pr28138-3.c b/ld/testsuite/ld-plugin/pr28138-3.c
new file mode 100644
index 00000000000..ec464947ee6
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138-3.c
@@ -0,0 +1,6 @@
+extern int a2(void);
+int
+a3(void)
+{
+  return 1 + a2();
+}
diff --git a/ld/testsuite/ld-plugin/pr28138-4.c b/ld/testsuite/ld-plugin/pr28138-4.c
new file mode 100644
index 00000000000..475701b2c5c
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138-4.c
@@ -0,0 +1,6 @@
+extern int a3(void);
+int
+a4(void)
+{
+  return 1 + a3();
+}
diff --git a/ld/testsuite/ld-plugin/pr28138-5.c b/ld/testsuite/ld-plugin/pr28138-5.c
new file mode 100644
index 00000000000..e24f86c363e
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138-5.c
@@ -0,0 +1,6 @@
+extern int a4(void);
+int
+a5(void)
+{
+  return 1 + a4();
+}
diff --git a/ld/testsuite/ld-plugin/pr28138-6.c b/ld/testsuite/ld-plugin/pr28138-6.c
new file mode 100644
index 00000000000..b5b938bdb21
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138-6.c
@@ -0,0 +1,6 @@
+extern int a5(void);
+int
+a6(void)
+{
+  return 1 + a5();
+}
diff --git a/ld/testsuite/ld-plugin/pr28138-7.c b/ld/testsuite/ld-plugin/pr28138-7.c
new file mode 100644
index 00000000000..4ef75bf0f0c
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138-7.c
@@ -0,0 +1,6 @@
+extern int a6(void);
+int
+a7(void)
+{
+  return 1 + a6();
+}
diff --git a/ld/testsuite/ld-plugin/pr28138.c b/ld/testsuite/ld-plugin/pr28138.c
new file mode 100644
index 00000000000..68252c9f382
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr28138.c
@@ -0,0 +1,20 @@
+#include <stdio.h>
+
+extern int a7(void);
+
+int
+a0(void)
+{
+  return 0;
+}
+
+int
+main()
+{
+  if (a7() == 7)
+    {
+      printf ("PASS\n");
+      return 0;
+    }
+  return 1;
+}

Patch

From 0ca3d329c6a0a53150746c01c6dbdce0999c944a Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 26 Jul 2021 05:59:55 -0700
Subject: [PATCH v2] bfd: Close the file descriptor if there is no archive fd

Close the file descriptor if there is no archive plugin file descriptor
to avoid running out of file descriptors on thin archives with many
archive members.

	PR ld/28138
	* plugin.c (bfd_plugin_close_file_descriptor): Close the file
	descriptor there is no archive plugin file descriptor.
---
 bfd/plugin.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/bfd/plugin.c b/bfd/plugin.c
index 6cfa2b66470..3bab8febe88 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -291,6 +291,14 @@  bfd_plugin_close_file_descriptor (bfd *abfd, int fd)
 	     && !bfd_is_thin_archive (abfd->my_archive))
 	abfd = abfd->my_archive;
 
+      /* Close the file descriptor if there is no archive plugin file
+	 descriptor.  */
+      if (abfd->archive_plugin_fd == -1)
+	{
+	  close (fd);
+	  return;
+	}
+
       abfd->archive_plugin_fd_open_count--;
       /* Dup the archive plugin file descriptor for later use, which
 	 will be closed by _bfd_archive_close_and_cleanup.  */
-- 
2.31.1