mmfile.d: MmFile destructor changed not to cal enforce #3529

sdegtiarev · 2015-08-06T20:57:45Z

Issue 14868:
MmFile(File,Mode,..) constructor caused the underlying file descriptor to be closed twice, in ~File() and ~MmFile(). Since MmFile destructor called enforce to check return status of close() system call, exception was thrown from inside GC.
The .close() call is safe and the only meaningful error is EBADF, when the descriptor is closed already, so I commented out the enforce part.

DmitryOlshansky · 2015-08-06T21:08:55Z

Please do not comment out code - we have full history with git anyway, otherwise - LGTM

sdegtiarev · 2015-08-07T14:21:53Z

Dmitry, what's going on? Looking at the error log, it's not related to the code change at all.

DmitryOlshansky · 2015-08-07T14:45:24Z

First of all we need to get you whitelisted. @braddr @MartinNowak

yebblies · 2015-08-10T00:51:14Z

@DmitryOlshansky Anyone with pull rights can add a user to the white-list here: https://auto-tester.puremagic.com/pulls.ghtml?projectid=1

DmitryOlshansky · 2015-08-10T06:51:39Z

std/mmfile.d

@@ -369,9 +369,13 @@ class MmFile
        }
        else version (Posix)
        {
+            /*


Just remove this code please. No commented-out outdated code in Phobos ;)

DmitryOlshansky · 2015-08-10T06:53:22Z

Dmitry, what's going on? Looking at the error log, it's not related to the code change at all.

That is documentation tester, not related to this pull. I whitelisted you and a couple of new folks, you should see the results soon.

DmitryOlshansky · 2015-08-11T16:33:17Z

@sdegtiarev - cool. One last thing - please squash these 2 commits to one and force-update this branch, thanks!

commented code removed

sdegtiarev · 2015-08-11T17:41:31Z

done
I hope, you understand that this is not a real fix. Two unrelated objects, File and MmFile, are still sharing file
descriptor and indirectly mapped memory, very bad idea.

On Tue, 8/11/15, Dmitry Olshansky notifications@github.com wrote:

Subject: Re: [phobos] mmfile.d: MmFile destructor changed not to cal enforce (#3529)
To: "D-Programming-Language/phobos" phobos@noreply.github.com
Cc: "sdegtiarev" sdegtiarev@yahoo.com
Date: Tuesday, August 11, 2015, 12:33 PM

@sdegtiarev - cool.
One last thing - please squash these 2 commits to one and
force-update this branch, thanks!

—
Reply to this email directly or view
it on GitHub.

DmitryOlshansky · 2015-08-11T21:05:28Z

I hope, you understand that this is not a real fix. Two unrelated objects, File and MmFile, are still sharing file descriptor and indirectly mapped memory, very bad idea.

Fixing bad design of std.mmfile is something to be addressed later. Anyhow I think having this stop-gap fix is good measure for now.

DmitryOlshansky · 2015-08-11T21:05:50Z

Auto-merge toggled on

mmfile.d: MmFile destructor changed not to cal enforce

sdegtiarev · 2015-08-13T16:10:38Z

Dmitry,
I'd like to suggest different approach to memory mapping. This is not a replacement of the generic one, but higher level
interface instead. It allows to map any value type to file and to handle it like regular in-memory object. The value remains persistent between invocations and may be shared between processes.
The code is in separate branch:
https://github.com/sdegtiarev/phobos/blob/persistent/std/persistent.d

Can you ask someone to take a look and review? I don't actually know yet how to contribute to std.phobos properly.

On Tue, 8/11/15, Dmitry Olshansky notifications@github.com wrote:

Subject: Re: [phobos] mmfile.d: MmFile destructor changed not to cal enforce (#3529)
To: "D-Programming-Language/phobos" phobos@noreply.github.com
Cc: "sdegtiarev" sdegtiarev@yahoo.com
Date: Tuesday, August 11, 2015, 5:05 PM

Merged #3529.

—
Reply to this email directly or view
it on GitHub.

DmitryOlshansky · 2015-08-13T17:28:24Z

This is not a replacement of the generic one, but a higher level interface instead

Yes, I believe that new high-level designs are what the doctor ordered.

https://github.com/sdegtiarev/phobos/blob/persistent/std/persistent.d Can you ask someone to take a look and review? I don't actually know yet how to contribute to std.phobos properly.

The concept looks great to me. One thing is that you might to check and forbid pointers via hasIndirections!T. Overall I like it, the name most likely gonig to change as persistence is more general then mmap-ing objects to files. Also - does it work for dynamic arrays? I think it absolutely should special case for T[] by far the most comon use case.

Speaking of Phobos inclusion - it's a long road and if you are up to it I can help you with the formal process of adding a new module to phobos. It's a lot of work and takes god-like patience to carry out. Expect no less then 3-4 months of waiting. Are you ready ? ;)

sdegtiarev · 2015-08-13T18:23:30Z

to check and forbid pointers via hasIndirections!T.
absolutely should special case for T[] by far the most comon use case.
Absolutely, this is what I'm working on. More generally, all reference types should be disabled,
including pointers, classes and whatever it could be. Do you think hasIndirections!T is sufficient
to filter out all of these? I'm reading std.traits for the third time trying to find out the proper traits.
As for Persistent!(T[]) dynamic array support, I meant it from the very beginning, but it tightly depends
on permissions, are we allowed to create the file, extend it, etc? In short, will be ready soon.

Are you ready ? ;)
Yes I am. I have a lot of patience and I don't rush, what I usually don't have, that's time, but next few months
seem to be easier, So why not?

btw, "doctor ordered" != "доктор прописал"
"доктор прописал" == "doctor prescribed" :)

On Thu, 8/13/15, Dmitry Olshansky notifications@github.com wrote:

Subject: Re: [phobos] mmfile.d: MmFile destructor changed not to cal enforce (#3529)
To: "D-Programming-Language/phobos" phobos@noreply.github.com
Cc: "sdegtiarev" sdegtiarev@yahoo.com
Date: Thursday, August 13, 2015, 1:28 PM

This is not a replacement of the generic one, but a higher level interface instead

Yes, I believe that new high-level designs are what the doctor ordered.

https://github.com/sdegtiarev/phobos/blob/persistent/std/persistent.d
Can you ask someone to take a look and review? I don't actually know yet how to contribute to std.phobos
properly.

The concept looks great to me. One thing is that you might to check and forbid pointers via hasIndirections!T.
Overall I like it, the name most likely gonig to change as persistence is more general then mmaping objects to files.
Also - does it work for dynamic arrays? I think it absolutely should special case for T[] by far the most comon use case.

Speaking of Phobos inclusion - it's a long road and if you are up to it I can help you with the formal process
of adding a new module to phobos. It's a lot of work and takes god-like patience to carry out. Expect no less then
3-4 months of waiting. Are you ready ? ;)

—
Reply to this email directly or view
it on GitHub.

DmitryOlshansky · 2015-08-14T05:04:58Z

Are you ready ? ;)
Yes I am. I have a lot of patience and I don't rush, what I usually don't have, that's time, but next few months
seem to be easier, So why not?

Great, looking forward to it. The first step would be making it std.experimental.mmap-some-name, opening a pull and starting a discussion on NG.

P. S. Now that you say it looked it up. Actually the idiom is "just what the doctor ordered". It might not be word for word for "то что доктор прописал" but has exactly the same meaning.

http://idioms.thefreedictionary.com/just+what+the+doctor+ordered

sdegtiarev · 2015-08-14T14:07:56Z

Great, I'l take a week-two then to tidy it up, there are some subtle points both in interface and impl., and will submit it to experimental.

On Fri, 8/14/15, Dmitry Olshansky notifications@github.com wrote:

Subject: Re: [phobos] mmfile.d: MmFile destructor changed not to cal enforce (#3529)
To: "D-Programming-Language/phobos" phobos@noreply.github.com
Cc: "sdegtiarev" sdegtiarev@yahoo.com
Date: Friday, August 14, 2015, 1:05 AM

Are you ready ? ;)

Yes I am. I have a lot of patience and I don't rush,
what I usually don't have, that's time, but next few
months

seem to be easier, So why not?

Great, looking forward to it. The first step would be
making it std.experimental.mmap-some-name, opening a pull
and starting a discussion on NG.

P. S. Now that you say it looked it up. Actually the
idiom is "just what the doctor ordered". It might
not be word for word for "то что доктор
прописал" but has exactly the same meaning.

http://idioms.thefreedictionary.com/just+what+the+doctor+ordered

—
Reply to this email directly or view
it on GitHub.

CyberShadow · 2015-09-01T07:08:18Z

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

CyberShadow · 2015-09-01T07:09:38Z

This fix is incorrect and dangerous. You've removed a potential InvalidMemoryOperationError, which is deadly but easy to detect, but retained the possibility of closing the file handle of a completely unrelated file, if that file handle has since been reused. With this change, the bug will always be silent and much harder to find the real cause of.

sdegtiarev · 2015-09-01T18:30:08Z

Absolutely. As I said, the right way would be to duplicate the file descriptor. This one was assumed as hot temporary fix.

sdegtiarev · 2015-09-01T18:35:54Z

Just not to forget, mmfile allows to create zero length file, but throws an exception when unmapping.
Also need to be fixed.

CyberShadow · 2015-09-02T01:55:59Z

but throws an exception when unmapping

When unmapping? I haven't seen this in the test I wrote: https://github.com/D-Programming-Language/phobos/pull/3619/files#diff-cb622bc0f28338bebabff9a7dd949706R680

sdegtiarev · 2015-09-02T03:10:59Z

munmap() is called from destructor, called by GC
Something like this:
// to create empty file
{ File(deleteme,"w"); }
auto m=new MmFile(deleteme);
assert(m.length == 0);
.....
... core.exception.InvalidMemoryOperationError@(0)
I have instrumented test with debug info, where it is seen that munmap(void*,0) is called (which always causes SIGSEGV or SIGBUS on *nix), however I have not access to that machine now. I will add the example tomorrow.

CyberShadow · 2015-09-02T03:12:29Z

I think that was caused by https://issues.dlang.org/show_bug.cgi?id=14994

Fix: CyberShadow@836be85

sdegtiarev · 2015-09-02T03:19:34Z

One more issue:

    // new file with size=3
    { File(deleteme,"w").write("123"); }

    // read-only shared memory of 64 bytes
    // the read-write mode would expand the file
    auto m=new MmFile(deleteme, MmFile.Mode.read, 64UL, null);
    assert(m.length == 64);
    //  however the file is still 3 bytes

in short, when file is opened in readOnly mode, it neglects actual file size. Tail bytes are not shared.

sdegtiarev · 2015-09-02T15:37:05Z

Oops, found something interesting (this is about zero-size file unmapping again)
Actually, munmap() is called in two places: destructor and ensureMapped(), which is called for every opSlice and, in case the file is empty, always tries to remap the memory.
So, the first error is thrown from munmap() from ensureMapped(), and another one during stack unwind from inside munmap() from ~this();
The first one is treated as regular exception and the second one produces the core.exception.InvalidMemoryOperationError@(0)
The code:

unittest {
try {
    { File(deleteme,"w"); } // create zero size file
    auto m=new MmFile(deleteme, MmFile.Mode.read, 0, null);
    assert(m.length == 0);
    m[0]; //ensureMapped() => index 0 is not mapped => munmap(data, 0) => throw

    writeln("done");
    remove(deleteme);
}
// ~this() => munmap(data, 0) => throw again
catch(Exception x)
{
    writeln("error");
}

neither of writeln() statements is ever reached

CyberShadow · 2015-09-03T09:22:54Z

Not sure I see the problem. 0-size maps are only supported on OS X 32-bit I think. m[0] is an out-of-bounds index so it should fail. And the InvalidMemoryOperationError should be fixed in #3619.

Please test with #3619 and see if there are any notable problems still (digger build 'master+phobos#3619').

sdegtiarev · 2015-09-03T13:04:08Z

There's a bunch of problems here. Exception thrown from inside GC and class destructor is one of them. As seen from the code, try/catch can't handle it.
More, the file size is essentially runtime parameter. From the perspective of the user of the class, if zero sized file was accidentally opened once, there is no safe way to leap out of the situation.
Nevertheless, the fix is quite simple, just to check size >0 when mmap'ing, under no circumstances the user wants to mmap zero bytes.

CyberShadow · 2015-09-03T13:15:37Z

Can you please tell me what concrete problems do you see with #3619 applied? Preferably unittest code that fails (with #3619 applied).

sdegtiarev · 2015-09-03T14:50:48Z

Retested, yes, with the fix everything is smooth. Constructor intercepts mmap() failure and throws.

sdegtiarev · 2015-09-03T15:52:36Z

@CyberShadow
found the concrete problem, see:

unittest // issue: read-only file is not extended
{
    import std.file : deleteme;
    import std.typecons : scoped;

    auto fn = deleteme ~ "-testing.txt";
    scope(exit) std.file.remove(fn);

    /// create new very short file
    { File(fn,"w").write("123"); }

    /** MmFile.Mode.read does not resize the file
     *    but mmap() still allocates full system page for mapping 
    **/
    auto m=new MmFile(fn, MmFile.Mode.read, 0x1000, null);
    auto i=cast(int[]) m[];
    // since the memory is valid, we can access the entire slice
    auto x=i[$-1];

    /** Trying to map more than one page
     *  The file is not resized again, and only one page is allocated
     *  Now, the last half of the slice has not valid mapping
    **/
    auto n=new MmFile(fn, MmFile.Mode.read, 0x1020, null);
    auto k=cast(int[]) n[];
    /// Bus error here
    auto y=k[$-1];
}

CyberShadow · 2015-09-04T10:27:10Z

@sdegtiarev I see, can you file that as an issue on Bugzilla?

sdegtiarev · 2015-09-04T21:45:23Z

@CyberShadow
Added: Issue 15013
https://issues.dlang.org/show_bug.cgi?id=15013

DmitryOlshansky reviewed Aug 10, 2015
View reviewed changes

mmfile.d: MmFile destructor changed not to cal enforce

4f3b3f4

commented code removed

sdegtiarev force-pushed the master branch from a631c32 to 4f3b3f4 Compare August 11, 2015 17:35

DmitryOlshansky added a commit that referenced this pull request Aug 11, 2015

Merge pull request #3529 from sdegtiarev/master

fb54328

mmfile.d: MmFile destructor changed not to cal enforce

DmitryOlshansky merged commit fb54328 into dlang:master Aug 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mmfile.d: MmFile destructor changed not to cal enforce #3529

mmfile.d: MmFile destructor changed not to cal enforce #3529

sdegtiarev commented Aug 6, 2015

DmitryOlshansky commented Aug 6, 2015

sdegtiarev commented Aug 7, 2015

DmitryOlshansky commented Aug 7, 2015

yebblies commented Aug 10, 2015

DmitryOlshansky Aug 10, 2015

DmitryOlshansky commented Aug 10, 2015

DmitryOlshansky commented Aug 11, 2015

sdegtiarev commented Aug 11, 2015

DmitryOlshansky commented Aug 11, 2015

DmitryOlshansky commented Aug 11, 2015

sdegtiarev commented Aug 13, 2015

DmitryOlshansky commented Aug 13, 2015

sdegtiarev commented Aug 13, 2015

DmitryOlshansky commented Aug 14, 2015

sdegtiarev commented Aug 14, 2015

CyberShadow commented Sep 1, 2015

CyberShadow commented Sep 1, 2015

sdegtiarev commented Sep 1, 2015

sdegtiarev commented Sep 1, 2015

CyberShadow commented Sep 2, 2015

sdegtiarev commented Sep 2, 2015

CyberShadow commented Sep 2, 2015

sdegtiarev commented Sep 2, 2015

sdegtiarev commented Sep 2, 2015

CyberShadow commented Sep 3, 2015

sdegtiarev commented Sep 3, 2015

CyberShadow commented Sep 3, 2015

sdegtiarev commented Sep 3, 2015

sdegtiarev commented Sep 3, 2015

CyberShadow commented Sep 4, 2015

sdegtiarev commented Sep 4, 2015

mmfile.d: MmFile destructor changed not to cal enforce #3529

mmfile.d: MmFile destructor changed not to cal enforce #3529

Conversation

sdegtiarev commented Aug 6, 2015

DmitryOlshansky commented Aug 6, 2015

sdegtiarev commented Aug 7, 2015

DmitryOlshansky commented Aug 7, 2015

yebblies commented Aug 10, 2015

DmitryOlshansky Aug 10, 2015

Choose a reason for hiding this comment

DmitryOlshansky commented Aug 10, 2015

DmitryOlshansky commented Aug 11, 2015

sdegtiarev commented Aug 11, 2015

DmitryOlshansky commented Aug 11, 2015

DmitryOlshansky commented Aug 11, 2015

sdegtiarev commented Aug 13, 2015

Can you ask someone to take a look and review? I don't actually know yet how to contribute to std.phobos properly.

DmitryOlshansky commented Aug 13, 2015

sdegtiarev commented Aug 13, 2015

DmitryOlshansky commented Aug 14, 2015

sdegtiarev commented Aug 14, 2015

Great, I'l take a week-two then to tidy it up, there are some subtle points both in interface and impl., and will submit it to experimental.

CyberShadow commented Sep 1, 2015

CyberShadow commented Sep 1, 2015

sdegtiarev commented Sep 1, 2015

sdegtiarev commented Sep 1, 2015

CyberShadow commented Sep 2, 2015

sdegtiarev commented Sep 2, 2015

CyberShadow commented Sep 2, 2015

sdegtiarev commented Sep 2, 2015

sdegtiarev commented Sep 2, 2015

CyberShadow commented Sep 3, 2015

sdegtiarev commented Sep 3, 2015

CyberShadow commented Sep 3, 2015

sdegtiarev commented Sep 3, 2015

sdegtiarev commented Sep 3, 2015

CyberShadow commented Sep 4, 2015

sdegtiarev commented Sep 4, 2015