code review 66590044: runtime/debug: add SetPanicOnFault

runtime/debug: add SetPanicOnFault

SetPanicOnFault allows recovery from unexpected memory faults.
This can be useful if you are using a memory-mapped file
or probing the address space of the current program.

[rsc, 2014-02-20 21:10:01]

Hello r (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[r, 2014-02-20 21:12:17]

LGTM

don't forget go1.3.text

[rsc, 2014-02-20 21:18:09]

*** Submitted as https://code.google.com/p/go/source/detail?r=b5eda189b974 ***

runtime/debug: add SetPanicOnFault

SetPanicOnFault allows recovery from unexpected memory faults.
This can be useful if you are using a memory-mapped file
or probing the address space of the current program.

LGTM=r
R=r
CC=golang-codereviews
https://codereview.appspot.com/66590044

[gobot, 2014-02-20 22:07:34]

This CL appears to have broken the darwin-386-cheney builder.

[dave_cheney.net, 2014-02-20 22:09:30]

Unrelated


On Fri, Feb 21, 2014 at 9:07 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the darwin-386-cheney builder.
>
>
> https://codereview.appspot.com/66590044/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[dvyukov, 2014-02-21 13:35:02]

I was thinking about the following interface:

SetPanicOnFaultRange(start *byte, size uintptr, on bool)

It solves the problem of granularity -- you are only interested in a particular
range, there is still nothing you can do with runtime data structures
corruption.
And it solves the problem of goroutine boundaries -- goroutines are ephemeral,
so any per-goroutine flags look suspicious.

I am also a bit concerned that we are using runtime/debug as a trash bin. We
actually want to use it for production, right?

[rsc, 2014-02-21 15:47:49]

On Fri, Feb 21, 2014 at 8:35 AM, <dvyukov@google.com> wrote:

> I was thinking about the following interface:
>
> SetPanicOnFaultRange(start *byte, size uintptr, on bool)
>
> It solves the problem of granularity -- you are only interested in a
> particular range, there is still nothing you can do with runtime data
> structures corruption.
> And it solves the problem of goroutine boundaries -- goroutines are
> ephemeral, so any per-goroutine flags look suspicious.
>

I think the goroutine part is a feature, not a bug. When you know you are
about to do something that might fault, you call SetPanicOnFault, and then
when you are done you restore the old setting. The goroutine that is
executing the thing that might fault is the one that needs the flag, not
the whole program. Being per-goroutine makes setting the flag free and
avoids changing the behavior of other goroutines. Also, since the garbage
collection runs on a g0 stack, being per-goroutine means that the flag will
never keep a bad garbage collection from crashing.

I don't believe the range adds anything terribly useful.


> I am also a bit concerned that we are using runtime/debug as a trash
> bin. We actually want to use it for production, right?
>

It's a bit unorganized as far as API goes - it's the runtime knobs - but I
don't think it's quite at the level of trash bin.

Russ

[dvyukov, 2014-02-22 11:01:33]

On 2014/02/21 15:47:49, rsc wrote:
> > I am also a bit concerned that we are using runtime/debug as a trash
> > bin. We actually want to use it for production, right?
> >
> 
> It's a bit unorganized as far as API goes - it's the runtime knobs - but I
> don't think it's quite at the level of trash bin.


But this function is not for debugging, it's for tuning runtime behavior. Why
are we adding such functions to debug?

[r, 2014-02-22 16:04:57]

It's for debugging.

On Sat, Feb 22, 2014 at 3:01 AM,  <dvyukov@google.com> wrote:
> On 2014/02/21 15:47:49, rsc wrote:
>>
>> > I am also a bit concerned that we are using runtime/debug as a trash
>> > bin. We actually want to use it for production, right?
>> >
>
>
>> It's a bit unorganized as far as API goes - it's the runtime knobs -
>
> but I
>>
>> don't think it's quite at the level of trash bin.
>
>
>
> But this function is not for debugging, it's for tuning runtime
> behavior. Why are we adding such functions to debug?
>
>
> https://codereview.appspot.com/66590044/

[albert.strasheim, 2014-02-22 18:18:39]

Hello

On 2014/02/20 21:18:09, rsc wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=b5eda189b974 ***
> runtime/debug: add SetPanicOnFault
> SetPanicOnFault allows recovery from unexpected memory faults.
> This can be useful if you are using a memory-mapped file
> or probing the address space of the current program.

It's been a while since I played with this, but last time I accessed an mmaped
file on a removable drive from a Go program on Linux and unplugged the drive,
I'm pretty sure I saw a SIGBUS.

From what I understand of this patch, it's only going to catch SIGSEGV?

Regards

Albert

[rsc, 2014-02-22 20:35:17]

SIGBUS too.

[dvyukov, 2014-02-23 09:49:34]

On Sat, Feb 22, 2014 at 4:04 PM, Rob Pike <r@golang.org> wrote:
> It's for debugging.

I still do not understand, because I though that it's for mmap-based
IO. But I trust you guys to handle this aspect.


> On Sat, Feb 22, 2014 at 3:01 AM,  <dvyukov@google.com> wrote:
>> On 2014/02/21 15:47:49, rsc wrote:
>>>
>>> > I am also a bit concerned that we are using runtime/debug as a trash
>>> > bin. We actually want to use it for production, right?
>>> >
>>
>>
>>> It's a bit unorganized as far as API goes - it's the runtime knobs -
>>
>> but I
>>>
>>> don't think it's quite at the level of trash bin.
>>
>>
>>
>> But this function is not for debugging, it's for tuning runtime
>> behavior. Why are we adding such functions to debug?
>>
>>
>> https://codereview.appspot.com/66590044/

[dvyukov, 2014-02-23 10:11:28]

On Fri, Feb 21, 2014 at 3:47 PM, Russ Cox <rsc@golang.org> wrote:
> On Fri, Feb 21, 2014 at 8:35 AM, <dvyukov@google.com> wrote:
>>
>> I was thinking about the following interface:
>>
>> SetPanicOnFaultRange(start *byte, size uintptr, on bool)
>>
>> It solves the problem of granularity -- you are only interested in a
>> particular range, there is still nothing you can do with runtime data
>> structures corruption.
>> And it solves the problem of goroutine boundaries -- goroutines are
>> ephemeral, so any per-goroutine flags look suspicious.
>
>
> I think the goroutine part is a feature, not a bug. When you know you are
> about to do something that might fault, you call SetPanicOnFault, and then
> when you are done you restore the old setting. The goroutine that is
> executing the thing that might fault is the one that needs the flag, not the
> whole program. Being per-goroutine makes setting the flag free and avoids
> changing the behavior of other goroutines. Also, since the garbage
> collection runs on a g0 stack, being per-goroutine means that the flag will
> never keep a bad garbage collection from crashing.
>
> I don't believe the range adds anything terribly useful.


1. If you access the memory in your own code, you wrap it into PanicOnFault.

2. If you need to hand off io.Reader/Writer to third party code, you
can wrap it into another Reader/Writer that sets/resets PanicOnFault.

3. If you need to hand off []byte/string to third party code, you do... what?
Copying is not an option, because it destroys the very purpose of using mmap
IO.
If you wrap the call into third party code into PanicOnFault, then (1)
you inevitably wrap lots of unrelated code, and (2) you don't actually
know on what goroutine the code will access the memory (think of
text/template lexer which does read on a different goroutine). I
understand that if the helper goroutine is not prepared to handle
panics, then PanicOnFault won't help as is. But it's quite orthogonal
-- if the memory access panics, there is a way to adopt the code to
handle the panic; and if the memory access crashes, there is not way
to handle it at all.

What do you think about this?

[rsc, 2014-02-23 15:44:15]

runtime/debug is poorly named. It is more like runtime/knobs. Too late to
change.

On Sun, Feb 23, 2014 at 5:11 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> 3. If you need to hand off []byte/string to third party code, you do...
> what?
>

Don't do this. Ordinary code expects that if i < len(x), x[i] cannot panic.
There is no reason to believe the code would be written to correctly handle
a panic during that evaluation. Code handling memory that might fault needs
to know this fact. You cannot hide it.

Russ

[dvyukov, 2014-02-24 05:57:20]

On Sun, Feb 23, 2014 at 7:44 PM, Russ Cox <rsc@golang.org> wrote:
> runtime/debug is poorly named. It is more like runtime/knobs. Too late to
> change.
>
> On Sun, Feb 23, 2014 at 5:11 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> 3. If you need to hand off []byte/string to third party code, you do...
>> what?
>
>
> Don't do this. Ordinary code expects that if i < len(x), x[i] cannot panic.
> There is no reason to believe the code would be written to correctly handle
> a panic during that evaluation. Code handling memory that might fault needs
> to know this fact. You cannot hide it.


I guess then you need to wrap PanicOnFault Reader into bufio.Reader,
because defer/recover are not cheap enough to be executed for every
few bytes. Is it the intention?

[minux1, 2014-02-24 06:09:32]

On Mon, Feb 24, 2014 at 12:56 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Sun, Feb 23, 2014 at 7:44 PM, Russ Cox <rsc@golang.org> wrote:
> > runtime/debug is poorly named. It is more like runtime/knobs. Too late to
> > change.
> >
> > On Sun, Feb 23, 2014 at 5:11 AM, Dmitry Vyukov <dvyukov@google.com>
> wrote:
> >>
> >> 3. If you need to hand off []byte/string to third party code, you do...
> >> what?
> > Don't do this. Ordinary code expects that if i < len(x), x[i] cannot
> panic.
> > There is no reason to believe the code would be written to correctly
> handle
> > a panic during that evaluation. Code handling memory that might fault
> needs
> > to know this fact. You cannot hide it.
>
> I guess then you need to wrap PanicOnFault Reader into bufio.Reader,
> because defer/recover are not cheap enough to be executed for every
> few bytes. Is it the intention?

What's a PanicOnFault reader? Did you mean a Reader backed by a mmap'ed
memory region?
If so, is bufio still needed? I think using mmap is precisely to avoid
double-buffering
(by both the user application and the kernel).

[dvyukov, 2014-02-24 06:12:20]

On Mon, Feb 24, 2014 at 10:09 AM, minux <minux.ma@gmail.com> wrote:
>
> On Mon, Feb 24, 2014 at 12:56 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> On Sun, Feb 23, 2014 at 7:44 PM, Russ Cox <rsc@golang.org> wrote:
>> > runtime/debug is poorly named. It is more like runtime/knobs. Too late
>> > to
>> > change.
>> >
>> > On Sun, Feb 23, 2014 at 5:11 AM, Dmitry Vyukov <dvyukov@google.com>
>> > wrote:
>> >>
>> >> 3. If you need to hand off []byte/string to third party code, you do...
>> >> what?
>> > Don't do this. Ordinary code expects that if i < len(x), x[i] cannot
>> > panic.
>> > There is no reason to believe the code would be written to correctly
>> > handle
>> > a panic during that evaluation. Code handling memory that might fault
>> > needs
>> > to know this fact. You cannot hide it.
>>
>> I guess then you need to wrap PanicOnFault Reader into bufio.Reader,
>> because defer/recover are not cheap enough to be executed for every
>> few bytes. Is it the intention?
>
> What's a PanicOnFault reader? Did you mean a Reader backed by a mmap'ed
> memory region?

Yes, I mean a reader that reads from mmap-ed region and sets/resets
PanicOnFault around reading.


> If so, is bufio still needed? I think using mmap is precisely to avoid
> double-buffering
> (by both the user application and the kernel).

"because defer/recover are not cheap enough to be executed for every few bytes"

you need to transform panic into error

[minux1, 2014-02-24 06:19:36]

On Mon, Feb 24, 2014 at 1:11 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Mon, Feb 24, 2014 at 10:09 AM, minux <minux.ma@gmail.com> wrote:
> > On Mon, Feb 24, 2014 at 12:56 AM, Dmitry Vyukov <dvyukov@google.com>
> wrote:
> >>
> >> On Sun, Feb 23, 2014 at 7:44 PM, Russ Cox <rsc@golang.org> wrote:
> >> > runtime/debug is poorly named. It is more like runtime/knobs. Too late
> >> > to
> >> > change.
> >> >
> >> > On Sun, Feb 23, 2014 at 5:11 AM, Dmitry Vyukov <dvyukov@google.com>
> >> > wrote:
> >> >>
> >> >> 3. If you need to hand off []byte/string to third party code, you
> do...
> >> >> what?
> >> > Don't do this. Ordinary code expects that if i < len(x), x[i] cannot
> >> > panic.
> >> > There is no reason to believe the code would be written to correctly
> >> > handle
> >> > a panic during that evaluation. Code handling memory that might fault
> >> > needs
> >> > to know this fact. You cannot hide it.
> >>
> >> I guess then you need to wrap PanicOnFault Reader into bufio.Reader,
> >> because defer/recover are not cheap enough to be executed for every
> >> few bytes. Is it the intention?
> >
> > What's a PanicOnFault reader? Did you mean a Reader backed by a mmap'ed
> > memory region?
>
> Yes, I mean a reader that reads from mmap-ed region and sets/resets
> PanicOnFault around reading.
>
> > If so, is bufio still needed? I think using mmap is precisely to avoid
> > double-buffering
> > (by both the user application and the kernel).
>
> "because defer/recover are not cheap enough to be executed for every few
> bytes"
> you need to transform panic into error
>
Doesn't a single panic on fault abort further operations on the same region?
Perhaps I don't understand your usage scenario in mind. Could you elaborate
a little?

I'm thinking about reading/writing mmapped files.

[dvyukov, 2014-02-24 06:23:41]

On Mon, Feb 24, 2014 at 10:19 AM, minux <minux.ma@gmail.com> wrote:
>
> On Mon, Feb 24, 2014 at 1:11 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> On Mon, Feb 24, 2014 at 10:09 AM, minux <minux.ma@gmail.com> wrote:
>> > On Mon, Feb 24, 2014 at 12:56 AM, Dmitry Vyukov <dvyukov@google.com>
>> > wrote:
>> >>
>> >> On Sun, Feb 23, 2014 at 7:44 PM, Russ Cox <rsc@golang.org> wrote:
>> >> > runtime/debug is poorly named. It is more like runtime/knobs. Too
>> >> > late
>> >> > to
>> >> > change.
>> >> >
>> >> > On Sun, Feb 23, 2014 at 5:11 AM, Dmitry Vyukov <dvyukov@google.com>
>> >> > wrote:
>> >> >>
>> >> >> 3. If you need to hand off []byte/string to third party code, you
>> >> >> do...
>> >> >> what?
>> >> > Don't do this. Ordinary code expects that if i < len(x), x[i] cannot
>> >> > panic.
>> >> > There is no reason to believe the code would be written to correctly
>> >> > handle
>> >> > a panic during that evaluation. Code handling memory that might fault
>> >> > needs
>> >> > to know this fact. You cannot hide it.
>> >>
>> >> I guess then you need to wrap PanicOnFault Reader into bufio.Reader,
>> >> because defer/recover are not cheap enough to be executed for every
>> >> few bytes. Is it the intention?
>> >
>> > What's a PanicOnFault reader? Did you mean a Reader backed by a mmap'ed
>> > memory region?
>>
>> Yes, I mean a reader that reads from mmap-ed region and sets/resets
>> PanicOnFault around reading.
>>
>> > If so, is bufio still needed? I think using mmap is precisely to avoid
>> > double-buffering
>> > (by both the user application and the kernel).
>>
>> "because defer/recover are not cheap enough to be executed for every few
>> bytes"
>> you need to transform panic into error
>
> Doesn't a single panic on fault abort further operations on the same region?
> Perhaps I don't understand your usage scenario in mind. Could you elaborate
> a little?
>
> I'm thinking about reading/writing mmapped files.

I also do not fully understand Russ' use case and intended usage of
PanicOnFault. Let's wait for him.

[dvyukov, 2014-02-24 07:51:14]

On Mon, Feb 24, 2014 at 10:23 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, Feb 24, 2014 at 10:19 AM, minux <minux.ma@gmail.com> wrote:
>>
>> On Mon, Feb 24, 2014 at 1:11 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>
>>> On Mon, Feb 24, 2014 at 10:09 AM, minux <minux.ma@gmail.com> wrote:
>>> > On Mon, Feb 24, 2014 at 12:56 AM, Dmitry Vyukov <dvyukov@google.com>
>>> > wrote:
>>> >>
>>> >> On Sun, Feb 23, 2014 at 7:44 PM, Russ Cox <rsc@golang.org> wrote:
>>> >> > runtime/debug is poorly named. It is more like runtime/knobs. Too
>>> >> > late
>>> >> > to
>>> >> > change.
>>> >> >
>>> >> > On Sun, Feb 23, 2014 at 5:11 AM, Dmitry Vyukov <dvyukov@google.com>
>>> >> > wrote:
>>> >> >>
>>> >> >> 3. If you need to hand off []byte/string to third party code, you
>>> >> >> do...
>>> >> >> what?
>>> >> > Don't do this. Ordinary code expects that if i < len(x), x[i] cannot
>>> >> > panic.
>>> >> > There is no reason to believe the code would be written to correctly
>>> >> > handle
>>> >> > a panic during that evaluation. Code handling memory that might fault
>>> >> > needs
>>> >> > to know this fact. You cannot hide it.
>>> >>
>>> >> I guess then you need to wrap PanicOnFault Reader into bufio.Reader,
>>> >> because defer/recover are not cheap enough to be executed for every
>>> >> few bytes. Is it the intention?
>>> >
>>> > What's a PanicOnFault reader? Did you mean a Reader backed by a mmap'ed
>>> > memory region?
>>>
>>> Yes, I mean a reader that reads from mmap-ed region and sets/resets
>>> PanicOnFault around reading.
>>>
>>> > If so, is bufio still needed? I think using mmap is precisely to avoid
>>> > double-buffering
>>> > (by both the user application and the kernel).
>>>
>>> "because defer/recover are not cheap enough to be executed for every few
>>> bytes"
>>> you need to transform panic into error
>>
>> Doesn't a single panic on fault abort further operations on the same region?
>> Perhaps I don't understand your usage scenario in mind. Could you elaborate
>> a little?
>>
>> I'm thinking about reading/writing mmapped files.
>
> I also do not fully understand Russ' use case and intended usage of
> PanicOnFault. Let's wait for him.


Additional bonus points for a full-fledged example that shows how to
properly use SetPanicOnFault for mmap IO.

[rsc, 2014-02-24 16:33:37]

On Mon, Feb 24, 2014 at 12:56 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> I guess then you need to wrap PanicOnFault Reader into bufio.Reader,
> because defer/recover are not cheap enough to be executed for every
> few bytes. Is it the intention?


it all depends on what you want to do with the code.

you are trying to pass off one of these sketchy buffers to code that
doesn't know it is handling a sketchy buffer. i think that is a bad idea,
fraught with peril.

in cmd/link i have a function that does all the copying from files into the
final executable image. it can do one defer before any of the copying
begins. this is possible because that code _knows_ it is dealing with
sketchy buffers. that is an okay requirement.

russ