Download Reference Manual
The Developer's Library for D
About Wiki Forums Source Search Contact

Ticket #1658 (closed defect: fixed)

Opened 10 years ago

Last modified 10 years ago

Conduit's destructor causes major problems with FileMap

Reported by: Deewiant Assigned to: kris
Priority: blocker Milestone: 0.99.9
Component: IO Version: trunk
Keywords: Cc:

Description

Conduit has a destructor which calls detach. FileMap and File are both Conduits, and FileMap contains a MappedFile which contains a File. FileMap.detach closes the MappedFile, which in turn tries to close the File it closes. But if the File's destructor has already been called, this fails catastrophically: we're calling a method of a destroyed and garbage collected object.

Here's a simple program which crashes all the time for me with the latest LDC (the problem has been reproduced with DMD 1.045 also):

import tango.io.device.FileMap;
void main() { new FileMap(__FILE__); }

Adding some strategic printf calls in Tango shows what's up:

$ ./arst
conduit DESTRUCTION of 1857347328
device detach of 1857347328
device detach of 1857347328 done
conduit DESTRUCTION of 1857347328 done
conduit DESTRUCTION of 1857347456
filemap detach of 1857347456: closing 1857351616
mappedfile close of 1857351616: closing 1857347328
zsh: segmentation fault  ./arst

The File got destroyed, then the FileMap got destroyed, which tries to do things with the just-destroyed File -> boom.

The only robust fix I can think of is to remove the destructor from Conduit. I'm not sure how problematic this would be.

Note that simply forcing all users to manually detach is not enough, since MappedFile.close can throw an exception from reset. Try the following code on a Posix system, for instance:

import tango.io.device.FileMap;

void main() { auto file = new FileMap("/dev/null"); file.detach; }

The FileMap is detached properly, yet because MappedFile.reset threw an exception, MappedFile.close never closed host and set it to null, and thus the double-free happens even though the detach was done properly.

If the above is fixed (e.g. by making MappedFile.close close host even when reset throws), an alternative "solution" to the problem is to simply document FileMap as requiring manual detaching or usage as a scope class.

Patch that fixes without removing Conduit's destructor:

Index: FileMap.d
===================================================================
--- FileMap.d	(revision 4656)
+++ FileMap.d	(working copy)
@@ -285,10 +285,12 @@
 
                 final void close ()
                 {
-                        reset;
-                        if (host)
-                            host.close;
-                        host = null;
+                        try reset;
+                        finally {
+                           if (host)
+                              host.close;
+                           host = null;
+                        }
                 }
 
                 /***************************************************************
@@ -302,7 +304,7 @@
                         //       On the other hand, this is NOT the case when the related 
                         //       file descriptor is closed.  This function unmaps explicitly.
                         if (base)
-                            if (munmap (base, size))
+                            if (munmap (base, size) && host)
                                 host.error;
 
                         base = null;    

Note the fix to not call host.error if it gets first detached (setting host to null) and later destroyed (calling reset again, but host is now null).

Change History

05/24/09 21:37:02 changed by kris

  • status changed from new to closed.
  • resolution set to fixed.

avoided the problem by using leveraging the distinction between close() and detach(). This needs more attention, since we can probably close the associated file right after the mapping is completed (and save a file handle)

05/24/09 21:39:34 changed by Deewiant

See r4695 for the fix applied.