Add Fiber's guard page in Posix as well by nemanja-boric-sociomantic · Pull Request #1698 · dlang/druntime

nemanja-boric-sociomantic · 2016-11-24T16:27:42Z

Windows Fiber implementation already uses the fiber stack guard page.
This brings the similar protection for mmaped fiber stacks, using
mprotect.

mihails-strasuns · 2016-11-28T11:49:12Z

src/core/thread.d

+                // protect end of stack
+                if ( mprotect(guard, PAGESIZE, PROT_NONE) == -1 )
+                    onOutOfMemoryError();
+            }


If valloc/malloc cases are not supported, I'd suggest putting explicit empty else clause with a comment about it (to

(to ... ? :-)

.. to better match previous static if block)

nemanja-boric-sociomantic · 2016-11-28T15:56:06Z

Updated with the else block and the comment saying that results are defined only for memory obtained by mmap

nemanja-boric-sociomantic · 2016-11-30T09:36:30Z

Not sure why DAutoTest failed:

make[1]: Leaving directory '/home/dtest/DAutoTest/work/repo/phobos'
for f in `find "/home/dtest/DAutoTest/work/repo/dlang.org/web/phobos-prerelease-verbatim" -iname '*.html'`; do mv $f `dirname $f`/`basename $f .html`.verbatim; done
mv /home/dtest/DAutoTest/work/repo/dlang.org/web/phobos-prerelease-verbatim/* /home/dtest/DAutoTest/work/repo/dlang.org/web/phobos-prerelease/
rm -r /home/dtest/DAutoTest/work/repo/dlang.org/web/phobos-prerelease-verbatim
fatal: 'refs/ae-sys-d-cache/website-2fa232357638ad439f82243dfc7f9d1c2f237c46-f798e1d288b93f2f15229c70de8009f8' - not a valid ref

PetarKirov · 2016-11-25T08:51:41Z

src/core/thread.d

+
+                // protect end of stack
+                if ( mprotect(guard, PAGESIZE, PROT_NONE) == -1 )
+                    onOutOfMemoryError();


Perhaps core.internal.abort is more appropriate here.

Yeah, but I was just following the module's style, which uses this for anything that can fail (see the same thing in Windows: https://github.com/nemanja-boric-sociomantic/druntime/blob/68bf497033575bdcb41a9392e72bdcf794c66016/src/core/thread.d#L4385-L4386)

Well, the other ones look like they allocate sth., b/c VirtualAlloc is used to change flags, and should be changed to fail instead of using onOutOfMemoryError.

OK, fair enough, will change to abort.

PetarKirov · 2016-12-01T06:45:48Z

The DAutoTest is about testing the documentation build, and so its failure is unrelated to your pull request.

MartinNowak · 2016-12-04T16:38:14Z

Maybe we should make the guard size configurable, including a size of 0, much as the stack size is configurable.
Not everyone might want to spent his address space and the (tiny) page mapping overhead, and a single page might not be enough for all memory corruption concerns.

MartinNowak · 2016-12-04T16:46:58Z

src/core/thread.d

+            {
+                // Mark end of stack
+                for ( ubyte* g = cast(ubyte*)guard; g < guard + PAGESIZE; g+= 32)
+                    g[0 .. 32] = cast(ubyte[]) "END OF FIBER -- END OF FIBER -- ";


Not acceptable to write to that page.
The point of a guard page is that it's not accessible, not backed by real memory, and causes a (page) fault when being accessed.

It is not a guarded at this point and writing recognizable string is the main purpose if this PR. It makes possible to easily debug stack overflow in a fiber by printing few bytes around rsp in gdb and checking if it resembles a guard string. Similar code has existed in original tango runtime and I have no idea how one is expected to work with heavy usage of fibers without it - pretty much everyone relies on it in daily workflow at Sociomantic :(

As Dicebot said, at this point, it is not guarded at this point, and in the lack of proper exception type, instead of SIGSEGV, this is very useful.

Not going to rediscuss why Exceptions for async failures are wrong, unwinding is hardly possible for those, and having the compiler assume that each instruction could throw requires complex unwind information or conservative (slow) code.
In any case, neither an exception nor a segfault for a stack overflow tells you much else that that the stack overflowed.

Sorry, I wasn't referring about the D exceptions, but the operating system's exception. For example, Windows builds will, this same runtime will, raise STATUS_GUARD_PAGE exception - which is very useful - it prevents you from silently corrupting your memory and it will tell you that your fiber's stack overflowed (if you're familiar with the runtime, though).

On POSIX, I'm not sure what else I can do here. The most reliable way to report the cause of the segmentation fault here is to install SIGSEGV handler, and to compare the siginfo_t.si_addr to see if this is done by fiber stack's overflow, eventually print info to stderr and die.

It is true that segmentation fault doesn't tell you why your program failed, but it least prevents you from silently corrupting other pages of the memory, making your program fail in completely unrelated code, just because some other fiber corrupted the memory. Sadly, this is not theoretical concern (and the code is already available on Windows), but this is something that we're seeing in practice every now and then.

However, even with the segmentation fault, if the core file is generated, you can load it in debugger and simply do x/s $sp-64 and see if the contents of the memory is just END OF FIBER. This is, btw, the reason why we're filling this page with data.

(I forgot to mention @MartinNowak to pass email filters :-) )

MartinNowak · 2016-12-04T16:48:31Z

src/core/thread.d

+
+                // protect end of stack
+                if ( mprotect(guard, PAGESIZE, PROT_NONE) == -1 )
+                    onOutOfMemoryError();


Well, the other ones look like they allocate sth., b/c VirtualAlloc is used to change flags, and should be changed to fail instead of using onOutOfMemoryError.

nemanja-boric-sociomantic · 2016-12-05T10:08:15Z

Maybe we should make the guard size configurable, including a size of 0, much as the stack size is configurable.
Not everyone might want to spent his address space and the (tiny) page mapping overhead, and a single page might not be enough for all memory corruption concerns.

Ok, sure. Do you prefer to do this via druntime config argument?

nemanja-boric-sociomantic · 2016-12-19T14:09:29Z

Updated with the:

Change onOutOfMemoryError to abort in case of mprotect failure
Allow configuring the size of the stack guard (on both Windows and Posix systems) with --DRT-stack-guard-size runtime option.

nemanja-boric-sociomantic · 2016-12-22T14:56:57Z

Ah, FreeBSD failed because by the time it got to testing, changelog.dd started containing merge confict.

nemanja-boric-sociomantic · 2016-12-22T15:08:54Z

Updated. @wilzbach could you advise if I got the new changelog format right?

wilzbach · 2016-12-22T15:13:33Z

Ah, FreeBSD failed because by the time it got to testing, changelog.dd started containing merge confict.

The idea is at that you will never have to rebase again due to changelog conflicts ;-)

dlang/phobos#4228

Updated. @wilzbach could you advise if I got the new changelog format right?

Yup - the idea was also that it's less complicated ;-)

nemanja-boric-sociomantic · 2016-12-22T15:14:13Z

Thanks!

MartinNowak · 2017-01-05T22:36:02Z

Sorry haven't looked at this for a while.

Why can't the guard page be write protected? That would be a lot better. In fact actually using a page is a huge overhead, not to mention the cost of writing to main memory.
Druntime config switches are fairly ugly and only intended for runtime "boot" configuration, doesn't seem necessary here, simply passing the guard size to the constructor of Fiber (like the stack size) should work, but might require to save the size somewhere.

nemanja-boric-sociomantic · 2017-01-09T13:04:52Z

Why can't the guard page be write protected? That would be a lot better. In fact actually using a page is a huge overhead, not to mention the cost of writing to main memory.

I'm not sure I follow this - the guard page is both write and read protected (see here: https://github.com/dlang/druntime/pull/1698/files#diff-8bb12ed976acf0a5132e877ec5a01ea8R4488)

I'll take a look later at other issues mentioned. Thanks!

nemanja-boric-sociomantic · 2017-04-27T12:56:28Z

Druntime config switches are fairly ugly and only intended for runtime "boot" configuration, doesn't seem necessary here, simply passing the guard size to the constructor of Fiber (like the stack size) should work, but might require to save the size somewhere.

I've updated the code so that this is configurable using Fiber's contructor's argument (with an ability to turn it off by setting 0). There wasn't need to save the size anywhere, since it's just counted in the stack's size (in POSIX's case), or it was deallocated automatically (like before in Window's case).

MartinNowak

Otherwise looks great.

MartinNowak · 2017-05-04T15:07:57Z

src/core/thread.d

+                {
+                    // Mark end of stack
+                    for ( ubyte* g = cast(ubyte*)guard; g < guard + guard_page_size; g+= 32)
+                        g[0 .. 32] = cast(ubyte[]) "END OF FIBER -- END OF FIBER -- ";


Here you're writing to the guard page which will cause it to actually get wired. Seems unnecessary, when the page is write protected below.

Yeah, the intention is to easily check if the stackoverflow has happened, simply by inspecting the memory pointed by sp in the debugger. I guess one could also check the mappings from /proc to confirm that the page is actually protected. If this writing is a deal breaker I would remove it.

MartinNowak · 2017-05-04T15:44:19Z

src/core/thread.d

     * Params:
     *  fn = The fiber function.
     *  sz = The stack size for this fiber.
+     *  guard_page_size = size of the guard page to trap fiber's stack


Change to guardPageSize to match our naming.

Will do. Thanks for the review!

I sent you an PR @Burgos.

nemanja-boric-sociomantic · 2017-05-05T14:09:11Z

Updated with your PR rebased.

Windows Fiber implementation already uses the fiber stack guard page. This brings the similar protection for mmaped fiber stacks, using mprotect.

This allows configuring the size or turning off completely the fiber's stack's guard page. It can be configured using new Fiber's constructor's parameter `guard_page_size`.

- no point in creating a dirty write protected page

Burgos · 2017-05-06T10:34:16Z

@MartinNowak I've pushed a small commit with a trivial tidying up the test case, if you don't mind.

Burgos · 2017-05-06T11:32:09Z

Green!

During the review of GitHub dlang#1698, the "END OF FIBRE" marker data was removed to avoid dirtying the guard page. Hence, the corresponding comment in the release notes doesn't apply anymore.

dnadlinger · 2017-05-06T16:25:47Z

changelog/fiber.dd

+page used for the Fiber's stack, the page is allocate which is protected for
+any kind of access. This will cause system to trap immediately on the fiber's
+stack overflow. If in debugger session, one can inspect contents of the memory
+before or after stack pointer and it can be seen if it contains END OF FIBER


This isn't true anymore, see #1821 for a fix.

During the review of GitHub dlang/druntime#1698, the "END OF FIBER" marker data was removed to avoid dirtying the guard page. Hence, the corresponding comment in the release notes doesn't apply anymore. See dlang/druntime#1821

nemanja-boric-sociomantic force-pushed the fiber-protection branch from 5707b74 to 0bf24b3 Compare November 24, 2016 17:08

mihails-strasuns reviewed Nov 28, 2016

View reviewed changes

nemanja-boric-sociomantic force-pushed the fiber-protection branch from 0bf24b3 to 68bf497 Compare November 28, 2016 15:55

PetarKirov reviewed Dec 1, 2016

View reviewed changes

nemanja-boric-sociomantic force-pushed the fiber-protection branch 2 times, most recently from 1d17e7c to f734164 Compare December 2, 2016 14:10

MartinNowak reviewed Dec 4, 2016

View reviewed changes

nemanja-boric-sociomantic force-pushed the fiber-protection branch from f734164 to 006855b Compare December 19, 2016 14:08

nemanja-boric-sociomantic force-pushed the fiber-protection branch 2 times, most recently from f9dad10 to c912db6 Compare December 19, 2016 14:23

nemanja-boric-sociomantic force-pushed the fiber-protection branch from c912db6 to dac1103 Compare December 22, 2016 15:08

nemanja-boric-sociomantic force-pushed the fiber-protection branch from dac1103 to 2458702 Compare April 27, 2017 12:54

MartinNowak suggested changes May 4, 2017

View reviewed changes

MartinNowak reviewed May 4, 2017

View reviewed changes

MartinNowak mentioned this pull request May 5, 2017

PR reviews and test nemanja-boric-sociomantic/druntime#1

Merged

MartinNowak approved these changes May 5, 2017

View reviewed changes

MartinNowak force-pushed the fiber-protection branch from 08d7552 to 35b1c80 Compare May 5, 2017 16:31

MartinNowak added the auto-merge label May 5, 2017

MartinNowak force-pushed the fiber-protection branch from 35b1c80 to 6a2b951 Compare May 6, 2017 08:56

dlang-bot removed the auto-merge label May 6, 2017

MartinNowak added the auto-merge label May 6, 2017

nemanja-boric-sociomantic force-pushed the fiber-protection branch from 6a2b951 to 98b907a Compare May 6, 2017 10:07

dlang-bot removed the auto-merge label May 6, 2017

nemanja-boric-sociomantic force-pushed the fiber-protection branch from 98b907a to 3ca3d41 Compare May 6, 2017 10:10

nemanja-boric-sociomantic and others added 5 commits May 6, 2017 12:13

Add Fiber's guard page in Posix as well

6ac9c09

Windows Fiber implementation already uses the fiber stack guard page. This brings the similar protection for mmaped fiber stacks, using mprotect.

Make Fiber's stack protection-page's size configurable

3e964f0

This allows configuring the size or turning off completely the fiber's stack's guard page. It can be configured using new Fiber's constructor's parameter `guard_page_size`.

change guard_page_size to camelCase

bd719ea

add test for Fiber guard page

3b7117b

do not write to guard page

a5c0dc2

- no point in creating a dirty write protected page

nemanja-boric-sociomantic force-pushed the fiber-protection branch 3 times, most recently from 0ca13da to e6c6ec3 Compare May 6, 2017 10:16

Simplify fiber guard page test

6be1794

nemanja-boric-sociomantic force-pushed the fiber-protection branch from e6c6ec3 to 6be1794 Compare May 6, 2017 10:20

MartinNowak merged commit a3505e7 into dlang:master May 6, 2017

dnadlinger mentioned this pull request May 6, 2017

Fixup fiber guard page changelog #1821

Merged

dnadlinger reviewed May 6, 2017

View reviewed changes

nemanja-boric-sociomantic deleted the fiber-protection branch May 8, 2017 13:04

nemanja-boric-sociomantic mentioned this pull request Jun 26, 2017

Fix the changelog for the Fiber's guard page. dlang/dlang.org#1774

Merged

Uh oh!

Comments

Conversation

nemanja-boric-sociomantic commented Nov 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemanja-boric-sociomantic commented Nov 28, 2016

Uh oh!

nemanja-boric-sociomantic commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Dec 1, 2016

Uh oh!

MartinNowak commented Dec 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemanja-boric-sociomantic commented Dec 5, 2016

Uh oh!

nemanja-boric-sociomantic commented Dec 19, 2016

Uh oh!

nemanja-boric-sociomantic commented Dec 22, 2016

Uh oh!

nemanja-boric-sociomantic commented Dec 22, 2016

Uh oh!

wilzbach commented Dec 22, 2016

Uh oh!

nemanja-boric-sociomantic commented Dec 22, 2016

Uh oh!

MartinNowak commented Jan 5, 2017

Uh oh!

nemanja-boric-sociomantic commented Jan 9, 2017

Uh oh!

nemanja-boric-sociomantic commented Apr 27, 2017

Uh oh!

MartinNowak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemanja-boric-sociomantic commented May 5, 2017

Uh oh!

Burgos commented May 6, 2017

Uh oh!

Burgos commented May 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!