[RFA/commit/procfs] /proc/.../map file descriptor leak

[RFA/commit/procfs] /proc/.../map file descriptor leak

[Joel Brobecker]

Hello,

This is on sparc-solaris.  I was trying to reproduce another error
that our testsuite occasionally reports (without much success :-().

I noticed while running the test repeatedly within the same debugger
process (that is: repeatedly doing "run; [...]; kill" instead of
starting a new debugger every time) that it would eventually cause
another error in the debugger. The debugger would be telling us that
it is unable to find a given thread in the procinfo list. In other
words, the debugger received an event for LWP=1 and then failed
to create the assocated thread element in the procinfo list.  After some
investgation, it turned out that this was because we were unable to open
the associated lwp file in the /proc filesystem, and that was because we
ran out of file descriptors!

Digging further, I found that we leak the file descriptor created
when opening the procfs map file. we create a cleanup routine to make
sure that the associated file descriptor gets closed, but we never
call the cleanup.

gdb/ChangeLog:

        * procfs.c (iterate_over_mappings): Call do_cleanups before
        returning.

Fixed thusly. Seems fairly straightforward, except that the cleanup
is only really needed when NEW_PROC_API is defined (basically, Solaris
except old versions).  But I think that #ifdef code is ugly, so I made
the cleanup run regardless...

I'll commit in a couple of days pending feedback...

---
 gdb/procfs.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index 871dd47..2a253a1 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -5217,6 +5217,7 @@ iterate_over_mappings (procinfo *pi, find_memory_region_ftype child_func,
   int funcstat;
   int map_fd;
   int nmap;
+  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
 #ifdef NEW_PROC_API
   struct stat sbuf;
 #endif
@@ -5254,8 +5255,12 @@ iterate_over_mappings (procinfo *pi, find_memory_region_ftype child_func,
 
   for (prmap = prmaps; nmap > 0; prmap++, nmap--)
     if ((funcstat = (*func) (prmap, child_func, data)) != 0)
-      return funcstat;
+      {
+	do_cleanups (cleanups);
+        return funcstat;
+      }
 
+  do_cleanups (cleanups);
   return 0;
 }
 
-- 
1.7.1

Re: [RFA/commit/procfs] /proc/.../map file descriptor leak

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Digging further, I found that we leak the file descriptor created
Joel> when opening the procfs map file. we create a cleanup routine to make
Joel> sure that the associated file descriptor gets closed, but we never
Joel> call the cleanup.

This tickled a memory:

    http://sourceware.org/ml/gdb-patches/2008-10/msg00683.html

I didn't touch iterate_over_mappings though, so the old patch is
complementary.

I think your patch is good.  I agree it is cleaner the way you wrote it.

Tom

Re: [RFA/commit/procfs] /proc/.../map file descriptor leak

[Joel Brobecker]

> This tickled a memory:
> 
>     http://sourceware.org/ml/gdb-patches/2008-10/msg00683.html
> 
> I didn't touch iterate_over_mappings though, so the old patch is
> complementary.

Argh. The part that changes the checks against zero might be a little
theoretical, and thus have no real effect, but the missing cleanups
are not. We should commit the patch. On solaris, I still cannot run
the testsuite (causing an awful machine crash), but I can run the AdaCore
testsuite. That's better than nothing.

> I think your patch is good.  I agree it is cleaner the way you wrote it.

Thanks for taking a look! I will commit momentarily.

-- 
Joel

Re: [RFA/commit/procfs] /proc/.../map file descriptor leak

[Tom Tromey]

Joel> Argh. The part that changes the checks against zero might be a little
Joel> theoretical, and thus have no real effect, but the missing cleanups
Joel> are not. We should commit the patch. On solaris, I still cannot run
Joel> the testsuite (causing an awful machine crash), but I can run the AdaCore
Joel> testsuite. That's better than nothing.

Did/could you try the patch?  I wrote it just based on reading the code,
I can't try it at all.  I can resend it if you want.

Tom

Re: [RFA/commit/procfs] /proc/.../map file descriptor leak

[Joel Brobecker]

> Joel> Argh. The part that changes the checks against zero might be a little
> Joel> theoretical, and thus have no real effect, but the missing cleanups
> Joel> are not. We should commit the patch. On solaris, I still cannot run
> Joel> the testsuite (causing an awful machine crash), but I can run the AdaCore
> Joel> testsuite. That's better than nothing.
> 
> Did/could you try the patch?  I wrote it just based on reading the
> code, I can't try it at all.  I can resend it if you want.

Yep, that was my intention - I meant to make that clear, but I am
juggling 3 different things at the same time, and quality is clearly
suffering :-(. In any case, I'm taking responsibility for testing it,
rebasing if necessary, and then getting it in for you.

-- 
Joel

Re: [RFA/commit/procfs] /proc/.../map file descriptor leak

[Tom Tromey]

Joel> Yep, that was my intention - I meant to make that clear, but I am
Joel> juggling 3 different things at the same time, and quality is clearly
Joel> suffering :-(. In any case, I'm taking responsibility for testing it,
Joel> rebasing if necessary, and then getting it in for you.

Thanks.
If you want some load off, I can at least rebase.. just let me know.

Tom

Re: [RFA/commit/procfs] /proc/.../map file descriptor leak

[Joel Brobecker]

> > This tickled a memory:
> > 
> >     http://sourceware.org/ml/gdb-patches/2008-10/msg00683.html
> > 
> > I didn't touch iterate_over_mappings though, so the old patch is
> > complementary.
> 
> Argh. The part that changes the checks against zero might be a little
> theoretical, and thus have no real effect, but the missing cleanups
> are not. We should commit the patch. On solaris, I still cannot run
> the testsuite (causing an awful machine crash), but I can run the AdaCore
> testsuite. That's better than nothing.

I tested the patch, and found no regression, so I checked it in.

> > I think your patch is good.  I agree it is cleaner the way you wrote it.
> 
> Thanks for taking a look! I will commit momentarily.

This one is in as well....

-- 
Joel