[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #15 from github-bugzi...@puremagic.com ---
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

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

github-bugzi...@puremagic.com changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #14 from github-bugzi...@puremagic.com ---
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

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

Vladimir Panteleev  changed:

   What|Removed |Added

 CC||thecybersha...@gmail.com

--- Comment #11 from Vladimir Panteleev  ---
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.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #12 from Vladimir Panteleev  ---
https://github.com/D-Programming-Language/phobos/pull/3529#issuecomment-136609619

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

Vladimir Panteleev  changed:

   What|Removed |Added

   Keywords||pull

--- Comment #13 from Vladimir Panteleev  ---
https://github.com/D-Programming-Language/phobos/pull/3619

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #10 from Dmitry Olshansky dmitry.o...@gmail.com ---

- 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=1repoid=3pullid=3527

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #9 from Sergei Degtiarev sdegtia...@yahoo.com ---
(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?

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #8 from Dmitry Olshansky dmitry.o...@gmail.com ---
 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.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #5 from Sergei Degtiarev sdegtia...@yahoo.com ---
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.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

Dmitry Olshansky dmitry.o...@gmail.com changed:

   What|Removed |Added

 CC||dmitry.o...@gmail.com

--- Comment #6 from Dmitry Olshansky dmitry.o...@gmail.com ---
~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.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #7 from Sergei Degtiarev sdegtia...@yahoo.com ---
(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.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #3 from Sergei Degtiarev sdegtia...@yahoo.com ---
The MmFile destructor is called after main() termination and throws exception
while trying to clean up the memory.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #4 from Maxim Fomin mxfo...@gmail.com ---
(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.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

Maxim Fomin mxfo...@gmail.com changed:

   What|Removed |Added

 CC||mxfo...@gmail.com

--- Comment #2 from Maxim Fomin mxfo...@gmail.com ---
Likely to be invalid, because dtor invokes gc code during collection.

File struct dtor calls detach(), which calls close(), which may call gc.

--

[Issue 14868] MmFile destructor seems to corrupt memory

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14868

--- Comment #1 from Sergei Degtiarev sdegtia...@yahoo.com ---
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

--