[PATCH] libbacktrace: fix fd leak tests on systems with extra descriptors

[PATCH] libbacktrace: fix fd leak tests on systems with extra descriptors

[Sergei Trofimovich]

From: Sergei Trofimovich <siarheit@google.com>

I noticed test failures when ran gcc test suite from under mc shell.
mc opens fd=9 and exposes it to child processes. As a result a few
tests failes:
    FAIL: b2test_buildid
    FAIL: btest_gnudebuglink
    FAIL: btest
    FAIL: btest_lto
    FAIL: btest_alloc
    FAIL: ctestg
    FAIL: ctesta
    FAIL: ctestg_alloc
    FAIL: ctesta_alloc
    FAIL: dwarf5
    FAIL: dwarf5_alloc

Instead of trying to close file descripts in range test polls for
first available file descriptor by creating it via dup(1).

libbacktrace/

	* btest.c (check_open_files): Use last free file descriptor as a
	signal for flie descriptor leak.
---
 libbacktrace/btest.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/libbacktrace/btest.c b/libbacktrace/btest.c
index 9f9c03babf3..d5cf321640c 100644
--- a/libbacktrace/btest.c
+++ b/libbacktrace/btest.c
@@ -458,22 +458,32 @@ test5 (void)
   return failures;
 }
 
+/* Peek at first free flie descriptior.  */
+
+static int probe_first_dree_fd (void) {
+  int fd;
+
+  fd = dup(1);
+  close(fd);
+
+  return fd;
+}
+
 /* Check that are no files left open.  */
 
 static void
-check_open_files (void)
+check_open_files (int last_free_fd)
 {
-  int i;
+  int fd;
+
+  fd = probe_first_dree_fd();
 
-  for (i = 3; i < 10; i++)
+  if (fd != last_free_fd)
     {
-      if (close (i) == 0)
-	{
-	  fprintf (stderr,
-		   "ERROR: descriptor %d still open after tests complete\n",
-		   i);
-	  ++failures;
-	}
+      fprintf (stderr,
+	       "ERROR: descriptor %d still open after tests complete\n",
+	       last_free_fd);
+      ++failures;
     }
 }
 
@@ -482,8 +492,11 @@ check_open_files (void)
 int
 main (int argc ATTRIBUTE_UNUSED, char **argv)
 {
+  int first_free_fd;
+
   state = backtrace_create_state (argv[0], BACKTRACE_SUPPORTS_THREADS,
 				  error_callback_create, NULL);
+  first_free_fd = probe_first_dree_fd ();
 
 #if BACKTRACE_SUPPORTED
   test1 ();
@@ -495,7 +508,7 @@ main (int argc ATTRIBUTE_UNUSED, char **argv)
 #endif
 #endif
 
-  check_open_files ();
+  check_open_files (first_free_fd);
 
   exit (failures ? EXIT_FAILURE : EXIT_SUCCESS);
 }
-- 
2.32.0

Re: [PATCH] libbacktrace: fix fd leak tests on systems with extra descriptors

[Ian Lance Taylor]

On Thu, Aug 12, 2021 at 3:34 PM Sergei Trofimovich via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Sergei Trofimovich <siarheit@google.com>
>
> I noticed test failures when ran gcc test suite from under mc shell.
> mc opens fd=9 and exposes it to child processes. As a result a few
> tests failes:
>     FAIL: b2test_buildid
>     FAIL: btest_gnudebuglink
>     FAIL: btest
>     FAIL: btest_lto
>     FAIL: btest_alloc
>     FAIL: ctestg
>     FAIL: ctesta
>     FAIL: ctestg_alloc
>     FAIL: ctesta_alloc
>     FAIL: dwarf5
>     FAIL: dwarf5_alloc
>
> Instead of trying to close file descripts in range test polls for
> first available file descriptor by creating it via dup(1).
>
> libbacktrace/
>
>         * btest.c (check_open_files): Use last free file descriptor as a
>         signal for flie descriptor leak.

This isn't a useful replacement, as this will pass as long as
libbacktrace closes the first file descriptor that it opens.  It won't
check whether libbacktrace left any other file descriptors open.

Perhaps at program startup we could fstat descriptors up to 10 and
record whether they are valid, and then skip those files in
check_open_files.

Ian

Re: [PATCH] libbacktrace: fix fd leak tests on systems with extra descriptors

[Sergei Trofimovich]

[-- Attachment #1.1: Type: text/plain, Size: 1531 bytes --]

On Thu, 12 Aug 2021 16:16:04 -0700
Ian Lance Taylor <iant@google.com> wrote:

> On Thu, Aug 12, 2021 at 3:34 PM Sergei Trofimovich via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > From: Sergei Trofimovich <siarheit@google.com>
> >
> > I noticed test failures when ran gcc test suite from under mc shell.
> > mc opens fd=9 and exposes it to child processes. As a result a few
> > tests failes:
> >     FAIL: b2test_buildid
> >     FAIL: btest_gnudebuglink
> >     FAIL: btest
> >     FAIL: btest_lto
> >     FAIL: btest_alloc
> >     FAIL: ctestg
> >     FAIL: ctesta
> >     FAIL: ctestg_alloc
> >     FAIL: ctesta_alloc
> >     FAIL: dwarf5
> >     FAIL: dwarf5_alloc
> >
> > Instead of trying to close file descripts in range test polls for
> > first available file descriptor by creating it via dup(1).
> >
> > libbacktrace/
> >
> >         * btest.c (check_open_files): Use last free file descriptor as a
> >         signal for flie descriptor leak.  
> 
> This isn't a useful replacement, as this will pass as long as
> libbacktrace closes the first file descriptor that it opens.  It won't
> check whether libbacktrace left any other file descriptors open.
> 
> Perhaps at program startup we could fstat descriptors up to 10 and
> record whether they are valid, and then skip those files in
> check_open_files.

Oh, great point! Completely missed it. Changed the patch to poll for present
file descriptors with fcntl(fd, F_GETFD) to compare before/after.

-- 

  Sergei

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: v2-0001-libbacktrace-fix-fd-leak-tests-on-systems-with-ex.patch --]
[-- Type: text/x-patch, Size: 2223 bytes --]

From dba67cc728d6be521f59a4c0d3abe7879de2db4c Mon Sep 17 00:00:00 2001
From: Sergei Trofimovich <siarheit@google.com>
Date: Thu, 12 Aug 2021 23:27:00 +0100
Subject: [PATCH v2] libbacktrace: fix fd leak tests on systems with extra
 descriptors

I noticed test failures when ran gcc test suite from under mc shell.
mc opens fd=9 and exposes it to child processes. As a result a few
tests failes:
    FAIL: b2test_buildid
    FAIL: btest_gnudebuglink
    FAIL: btest
    FAIL: btest_lto
    FAIL: btest_alloc
    FAIL: ctestg
    FAIL: ctesta
    FAIL: ctestg_alloc
    FAIL: ctesta_alloc
    FAIL: dwarf5
    FAIL: dwarf5_alloc

Instead of trying to close file descripts in range test polls for
their presence with with fcntl(fd, F_GETFD) before and after test.

libbacktrace/

	* btest.c (check_open_files): Use fcntl to poll for file
	descriptor presence.
---
 libbacktrace/btest.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/libbacktrace/btest.c b/libbacktrace/btest.c
index 9f9c03babf3..c08c1540fa6 100644
--- a/libbacktrace/btest.c
+++ b/libbacktrace/btest.c
@@ -34,6 +34,7 @@ POSSIBILITY OF SUCH DAMAGE.  */
    libbacktrace library.  */
 
 #include <assert.h>
+#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -458,6 +459,18 @@ test5 (void)
   return failures;
 }
 
+#define MAX_FDS 10
+static int fd_states[MAX_FDS];
+
+static void
+store_open_files (void)
+{
+  int i;
+
+  for (i = 0; i < MAX_FDS; i++)
+    fd_states[i] = fcntl (i, F_GETFD);
+}
+
 /* Check that are no files left open.  */
 
 static void
@@ -465,9 +478,9 @@ check_open_files (void)
 {
   int i;
 
-  for (i = 3; i < 10; i++)
+  for (i = 0; i < MAX_FDS; i++)
     {
-      if (close (i) == 0)
+      if (fcntl (i, F_GETFD) != fd_states[i])
 	{
 	  fprintf (stderr,
 		   "ERROR: descriptor %d still open after tests complete\n",
@@ -484,6 +497,7 @@ main (int argc ATTRIBUTE_UNUSED, char **argv)
 {
   state = backtrace_create_state (argv[0], BACKTRACE_SUPPORTS_THREADS,
 				  error_callback_create, NULL);
+  store_open_files ();
 
 #if BACKTRACE_SUPPORTED
   test1 ();
-- 
2.32.0


[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 981 bytes --]

Re: [PATCH] libbacktrace: fix fd leak tests on systems with extra descriptors

[Ian Lance Taylor]

On Fri, Aug 13, 2021 at 12:05 AM Sergei Trofimovich <slyich@gmail.com> wrote:
>
> On Thu, 12 Aug 2021 16:16:04 -0700
> Ian Lance Taylor <iant@google.com> wrote:
>
> > On Thu, Aug 12, 2021 at 3:34 PM Sergei Trofimovich via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > From: Sergei Trofimovich <siarheit@google.com>
> > >
> > > I noticed test failures when ran gcc test suite from under mc shell.
> > > mc opens fd=9 and exposes it to child processes. As a result a few
> > > tests failes:
> > >     FAIL: b2test_buildid
> > >     FAIL: btest_gnudebuglink
> > >     FAIL: btest
> > >     FAIL: btest_lto
> > >     FAIL: btest_alloc
> > >     FAIL: ctestg
> > >     FAIL: ctesta
> > >     FAIL: ctestg_alloc
> > >     FAIL: ctesta_alloc
> > >     FAIL: dwarf5
> > >     FAIL: dwarf5_alloc
> > >
> > > Instead of trying to close file descripts in range test polls for
> > > first available file descriptor by creating it via dup(1).
> > >
> > > libbacktrace/
> > >
> > >         * btest.c (check_open_files): Use last free file descriptor as a
> > >         signal for flie descriptor leak.
> >
> > This isn't a useful replacement, as this will pass as long as
> > libbacktrace closes the first file descriptor that it opens.  It won't
> > check whether libbacktrace left any other file descriptors open.
> >
> > Perhaps at program startup we could fstat descriptors up to 10 and
> > record whether they are valid, and then skip those files in
> > check_open_files.
>
> Oh, great point! Completely missed it. Changed the patch to poll for present
> file descriptors with fcntl(fd, F_GETFD) to compare before/after.

I believe that we currently run some of the tests on Windows systems.
Do you know if fcntl(fd, F_GETFD) will work there?

Ian

Re: [PATCH] libbacktrace: fix fd leak tests on systems with extra descriptors

[Sergei Trofimovich]

On Fri, 13 Aug 2021 09:52:53 -0700
Ian Lance Taylor <iant@google.com> wrote:

> On Fri, Aug 13, 2021 at 12:05 AM Sergei Trofimovich <slyich@gmail.com> wrote:
> >
> > On Thu, 12 Aug 2021 16:16:04 -0700
> > Ian Lance Taylor <iant@google.com> wrote:
> >  
> > > On Thu, Aug 12, 2021 at 3:34 PM Sergei Trofimovich via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:  
> > > >
> > > > From: Sergei Trofimovich <siarheit@google.com>
> > > >
> > > > I noticed test failures when ran gcc test suite from under mc shell.
> > > > mc opens fd=9 and exposes it to child processes. As a result a few
> > > > tests failes:
> > > >     FAIL: b2test_buildid
> > > >     FAIL: btest_gnudebuglink
> > > >     FAIL: btest
> > > >     FAIL: btest_lto
> > > >     FAIL: btest_alloc
> > > >     FAIL: ctestg
> > > >     FAIL: ctesta
> > > >     FAIL: ctestg_alloc
> > > >     FAIL: ctesta_alloc
> > > >     FAIL: dwarf5
> > > >     FAIL: dwarf5_alloc
> > > >
> > > > Instead of trying to close file descripts in range test polls for
> > > > first available file descriptor by creating it via dup(1).
> > > >
> > > > libbacktrace/
> > > >
> > > >         * btest.c (check_open_files): Use last free file descriptor as a
> > > >         signal for flie descriptor leak.  
> > >
> > > This isn't a useful replacement, as this will pass as long as
> > > libbacktrace closes the first file descriptor that it opens.  It won't
> > > check whether libbacktrace left any other file descriptors open.
> > >
> > > Perhaps at program startup we could fstat descriptors up to 10 and
> > > record whether they are valid, and then skip those files in
> > > check_open_files.  
> >
> > Oh, great point! Completely missed it. Changed the patch to poll for present
> > file descriptors with fcntl(fd, F_GETFD) to compare before/after.  
> 
> I believe that we currently run some of the tests on Windows systems.
> Do you know if fcntl(fd, F_GETFD) will work there?

Ah, it does not. At least mingw does not expose fcntl. I'll look
at possible equivalent on it and send another version.

-- 

  Sergei