Bug 14868 – MmFile destructor seems to corrupt memory

The following line of code: new MmFile(File("file.map","w+"), MmFile.Mode.readWriteNew, 1024, null); seems to confuse the GC: core.exception.InvalidMemoryOperationError@(0) Other constructor overloads, like: new MmFile("file.map", MmFile.Mode.readWriteNew, 1024, null); seem to work fine

Likely to be invalid, because dtor invokes gc code during collection. File struct dtor calls detach(), which calls close(), which may call gc.

The MmFile destructor is called after main() termination and throws exception while trying to clean up the memory.

(In reply to Sergei Degtiarev from comment #3) > The MmFile destructor is called after main() termination and throws > exception while trying to clean up the memory. Sounds like class dtor invokes gc. Because current GC implementation is not reenterant, such code in not supported.

MmFile class destructor makes only two system calls: ~this() { unmap(); errnoEnforce(fd == -1 || fd <= 2 || .close(fd) != -1, "Could not close handle"); } and the second one, .close(fd), fails with EBADF errno, obviously the file descriptor is already closed. Somehow GC converts descriptive error message into general InvalidMemoryOperationError. What I found, if the File is created as independent instance, it is always deleted first, so the file descriptor is closed once in File destructor and after that is tried to be closed in MmFile destructor resulting to an exception. I would recommend (at least as temporary fix for POSIX) to exclude .close() result check, the only meaningful error returned is due to close() duplicate call, which is harmless. Also a question, how it happens that File destructor is always called before MmFile one? I tried all possible combinations to create them with same result.

~this() { unmap(); errnoEnforce(fd == -1 || fd <= 2 || .close(fd) != -1, "Could not close handle"); } enforce internally throws exception so calling it in finalizer is baaad idea with current non-reentrant GC.

(In reply to Dmitry Olshansky from comment #6) > enforce internally throws exception so calling it in finalizer is baaad idea > with current non-reentrant GC. That's why I suggest to exclude the check. Can somebody fix it? The class is currently unusable.

> That's why I suggest to exclude the check. Can somebody fix it? Pull requests are welcome from anybody. >The class is currently unusable. I'm surprised it even works. std.mmfile is a port of some C++ library to D1 done ages ago. It was gradually updated to compile with the latest compiler but that's pretty much it.

(In reply to Dmitry Olshansky from comment #8) > Pull requests are welcome from anybody. Ok, good, I can do this. However I'm barely three days on the forum and don't know which way to go. I see at least three variants: - straightforward, to throw out enforce, it will work safely, guarantee - at C level, we can duplicate the file descriptor - from designer's perspective, I'd simply remove this excess constructor overload. Since we don't control lifetime of these two separate instances, File and MmFile, they should not share same resource, file descriptor. But won't this impact existing code? (if there is any) Besides that, I can't code neither test it for systems other than POSIX ones. Is it normal to live with two different implementations?

- straightforward, to throw out enforce, it will work safely, guarantee Seems like the way to go. I'd think of it as hotfix, it's apparent that file descriptor may already be closed by File object itself. Ideally we'd have more sensible MM-file IO and/or virtual memory management module, preferably in druntime. > Besides that, I can't code neither test it for systems other than POSIX ones. Is it normal to live with two different implementations? There is n auto-tester for that. All pulls are tested on all systems. See e.g. https://auto-tester.puremagic.com/pull-history.ghtml?projectid=1&repoid=3&pullid=3527

The problem here is that MmFile does not retain a copy of the File. As such, the File will be closed at the end of that statement, as it was a temporary that was never copied anywhere. The correct fix would be to add a private File field to MmFile and make it save a copy of the File parameter.

Commit pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/1fc0fa1fa9c9e76a77af63114c7e781651a2d4de fix Issue 14868 - MmFile destructor seems to corrupt memory

Commit pushed to stable at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/1fc0fa1fa9c9e76a77af63114c7e781651a2d4de fix Issue 14868 - MmFile destructor seems to corrupt memory