code review 139360044: syscall: pin allocated C string across call to Syscall

syscall: keep allocated C string live across call to Syscall

Given:

        p := alloc()
        fn_taking_ptr(p)

p is NOT recorded as live at the call to fn_taking_ptr:
it's not needed by the code following the call.
p was passed to fn_taking_ptr, and fn_taking_ptr must keep
it alive as long as it needs it.
In practice, fn_taking_ptr will keep its own arguments live
for as long as the function is executing.

But if instead you have:

        p := alloc()
        i := uintptr(unsafe.Pointer(p))
        fn_taking_int(i)

p is STILL NOT recorded as live at the call to fn_taking_int:
it's not needed by the code following the call.
fn_taking_int is responsible for keeping its own arguments
live, but fn_taking_int is written to take an integer, so even
though fn_taking_int does keep its argument live, that argument
does not keep the allocated memory live, because the garbage
collector does not dereference integers.

The shorter form:

        p := alloc()
        fn_taking_int(uintptr(unsafe.Pointer(p)))

and the even shorter form:

        fn_taking_int(uintptr(unsafe.Pointer(alloc())))

are both the same as the 3-line form above.

syscall.Syscall is like fn_taking_int: it is written to take a list
of integers, and yet those integers are sometimes pointers.
If there is no other copy of those pointers being kept live,
the memory they point at may be garbage collected during
the call to syscall.Syscall.

This is happening on Solaris: for whatever reason, the timing
is such that the garbage collector manages to free the string
argument to the open(2) system call before the system call
has been invoked.

Change the system call wrappers to insert explicit references
that will keep the allocations alive in the original frame
(and therefore preserve the memory) until after syscall.Syscall
has returned.

Should fix Solaris flakiness.

This is not a problem for cgo, because cgo wrappers have
correctly typed arguments.

[rsc, 2014-09-08 18:57:06]

Hello iant@golang.org (cc: aram@mgk.ro, dvyukov@google.com,
golang-codereviews@googlegroups.com, khr@golang.org, r@golang.org,
rlh@golang.org),

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

[iant, 2014-09-08 19:05:37]

LGTM

Does this handle every case, though?  What about something like syscall.Fstat(0,
&s) where s is not referenced after the call?  This doesn't make sense, of
course, but is it possible that s would be freed before the actual entry into
Syscall, causing the syscall to potentially corrupt memory that has been
allocated to some other purpose?  If that can't happen, then why is that
different from the string case?

[khr, 2014-09-08 19:09:02]

LGTM.

What method did you use to find all of these?  And/or: how do we know there are
not more?

Is this bug in 1.3?

[bradfitz, 2014-09-08 19:10:10]

Keith, most look like they were done via the Perl scripts modified in this CL
too.

[rsc, 2014-09-08 19:27:35]

On Mon, Sep 8, 2014 at 3:05 PM, <iant@golang.org> wrote:

> LGTM
>
> Does this handle every case, though?  What about something like
> syscall.Fstat(0, &s) where s is not referenced after the call?  This
> doesn't make sense, of course, but is it possible that s would be freed
> before the actual entry into Syscall, causing the syscall to potentially
> corrupt memory that has been allocated to some other purpose?  If that
> can't happen, then why is that different from the string case?
>

On Mon, Sep 8, 2014 at 3:09 PM, <khr@golang.org> wrote:

> LGTM.
>
> What method did you use to find all of these?  And/or: how do we know
> there are not more?
>
> Is this bug in 1.3?
>

cmd/gc/plive.c treats every return instruction as a read of all the input
and output parameters. syscall.Fstat's frame will keep the value &s alive
as a pointer, and so the garbage collector will trace it and keep what it
points at. Any typed system call wrapper will keep its arguments alive in
this way, even though it might make calls to the untyped syscall.Syscall
inside.

The problem we have identified is when a typed wrapper does an allocation
in its body and then does nothing with the pointer to that allocation
except pass it to syscall.Syscall as a uintptr. In that case, there is
nothing keeping the local allocation alive across the syscall.Syscall,
because it is just in a dead local variable. If the wrappers had been
written to use an extra function call around syscall.Syscall, the problem
would have been avoided too.

To reinforce the point, forkExec calls BytePtrFromString a handful of
times, but then it passes those values as pointers (not uintptrs)
to forkAndExecInChild, so the forkAndExecInChild frame will keep their
memory alive, so there's no need for calls to use in forkExec.

As another example, if instead of writing Fstat(0, &s) you wrote
syscall.Syscall(SYS_FSTAT, 0, uintptr(unsafe.Pointer(&s))), that would be
okay, because &s would not escape there, so the compiler would leave s on
the stack, so it can't be freed. You really need a variable-sized
allocation or one that has other references to it from off stack, or else
it will be on the stack and be okay.

I believe the only system call wrappers that allocate inside the wrapper
are the ones turning Go strings into C strings, so I looked for functions
that called both BytePtrFromString and Syscall. I found a few written by
hand and all the auto-generated ones.

Any other instances of this bug would need a function that allocates memory
and then does nothing with it but pass it to another function as type
uintptr. The only function I can think of that takes pointers as uintptrs
is Syscall. I guess we could look at all the calls to Syscall to see if we
can find nearby allocations done with functions other than
BytePtrFromString. At first glance I don't see any.

This is plausibly a bug in 1.3, although I'd want to see an actual report
of it in the wild before bothering to issue a point release. So far we
haven't even seen it at tip except on Solaris.

Russ

[rsc, 2014-09-08 19:28:10]

Yes, all the z files were regenerated by editing the perl scripts and
running 'mkall.sh -syscalls'.

[khr1, 2014-09-08 19:32:46]

On Mon, Sep 8, 2014 at 12:27 PM, Russ Cox <rsc@golang.org> wrote:

> On Mon, Sep 8, 2014 at 3:05 PM, <iant@golang.org> wrote:
>
> LGTM
>>
>> Does this handle every case, though?  What about something like
>> syscall.Fstat(0, &s) where s is not referenced after the call?  This
>> doesn't make sense, of course, but is it possible that s would be freed
>> before the actual entry into Syscall, causing the syscall to potentially
>> corrupt memory that has been allocated to some other purpose?  If that
>> can't happen, then why is that different from the string case?
>>
>
> On Mon, Sep 8, 2014 at 3:09 PM, <khr@golang.org> wrote:
>
>> LGTM.
>>
>> What method did you use to find all of these?  And/or: how do we know
>> there are not more?
>>
>> Is this bug in 1.3?
>>
>
> cmd/gc/plive.c treats every return instruction as a read of all the input
> and output parameters. syscall.Fstat's frame will keep the value &s alive
> as a pointer, and so the garbage collector will trace it and keep what it
> points at. Any typed system call wrapper will keep its arguments alive in
> this way, even though it might make calls to the untyped syscall.Syscall
> inside.
>
> The problem we have identified is when a typed wrapper does an allocation
> in its body and then does nothing with the pointer to that allocation
> except pass it to syscall.Syscall as a uintptr. In that case, there is
> nothing keeping the local allocation alive across the syscall.Syscall,
> because it is just in a dead local variable. If the wrappers had been
> written to use an extra function call around syscall.Syscall, the problem
> would have been avoided too.
>
> To reinforce the point, forkExec calls BytePtrFromString a handful of
> times, but then it passes those values as pointers (not uintptrs)
> to forkAndExecInChild, so the forkAndExecInChild frame will keep their
> memory alive, so there's no need for calls to use in forkExec.
>
> As another example, if instead of writing Fstat(0, &s) you wrote
> syscall.Syscall(SYS_FSTAT, 0, uintptr(unsafe.Pointer(&s))), that would be
> okay, because &s would not escape there, so the compiler would leave s on
> the stack, so it can't be freed. You really need a variable-sized
> allocation or one that has other references to it from off stack, or else
> it will be on the stack and be okay.
>
> I believe the only system call wrappers that allocate inside the wrapper
> are the ones turning Go strings into C strings, so I looked for functions
> that called both BytePtrFromString and Syscall. I found a few written by
> hand and all the auto-generated ones.
>
> Any other instances of this bug would need a function that allocates
> memory and then does nothing with it but pass it to another function as
> type uintptr. The only function I can think of that takes pointers as
> uintptrs is Syscall. I guess we could look at all the calls to Syscall to
> see if we can find nearby allocations done with functions other than
> BytePtrFromString. At first glance I don't see any.
>
> This is plausibly a bug in 1.3, although I'd want to see an actual report
> of it in the wild before bothering to issue a point release. So far we
> haven't even seen it at tip except on Solaris.
>
>
Makes sense, but we should be sure to include this fix in 1.3.2 if we have
other reasons to issue such a release.


> Russ
>

[aram, 2014-09-08 19:55:19]

LGTM

Happy that the Solaris port exposed a bug in all the other platforms.

[rlh, 2014-09-08 20:03:41]

On 2014/09/08 19:55:19, aram wrote:
> LGTM
> 
> Happy that the Solaris port exposed a bug in all the other platforms.

LGTM

That said, while this keeps the pointer alive it does not pin it as the CL title
suggests. It merely relies on the fact that the current GC implementation does
not move any objects. 1.4 and 1.5 will not be moving objects so we are good for
at least that time frame.

[rsc, 2014-09-08 20:59:06]

Thanks. I am still internalizing that what GC people mean by 'pin' is not
what I mean. I changed the CL subject line to

syscall: keep allocated C string live across call to Syscall

[rsc, 2014-09-08 21:00:06]

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

syscall: keep allocated C string live across call to Syscall

Given:

        p := alloc()
        fn_taking_ptr(p)

p is NOT recorded as live at the call to fn_taking_ptr:
it's not needed by the code following the call.
p was passed to fn_taking_ptr, and fn_taking_ptr must keep
it alive as long as it needs it.
In practice, fn_taking_ptr will keep its own arguments live
for as long as the function is executing.

But if instead you have:

        p := alloc()
        i := uintptr(unsafe.Pointer(p))
        fn_taking_int(i)

p is STILL NOT recorded as live at the call to fn_taking_int:
it's not needed by the code following the call.
fn_taking_int is responsible for keeping its own arguments
live, but fn_taking_int is written to take an integer, so even
though fn_taking_int does keep its argument live, that argument
does not keep the allocated memory live, because the garbage
collector does not dereference integers.

The shorter form:

        p := alloc()
        fn_taking_int(uintptr(unsafe.Pointer(p)))

and the even shorter form:

        fn_taking_int(uintptr(unsafe.Pointer(alloc())))

are both the same as the 3-line form above.

syscall.Syscall is like fn_taking_int: it is written to take a list
of integers, and yet those integers are sometimes pointers.
If there is no other copy of those pointers being kept live,
the memory they point at may be garbage collected during
the call to syscall.Syscall.

This is happening on Solaris: for whatever reason, the timing
is such that the garbage collector manages to free the string
argument to the open(2) system call before the system call
has been invoked.

Change the system call wrappers to insert explicit references
that will keep the allocations alive in the original frame
(and therefore preserve the memory) until after syscall.Syscall
has returned.

Should fix Solaris flakiness.

This is not a problem for cgo, because cgo wrappers have
correctly typed arguments.

LGTM=iant, khr, aram, rlh
R=iant, khr, bradfitz, aram, rlh
CC=dvyukov, golang-codereviews, r
https://codereview.appspot.com/139360044

[dfc, 2014-09-08 22:18:02]

Thanks for digging into this and finding a solution for Solaris Russ. I
appreciate the effort you put into this.
On 9 Sep 2014 06:59, "Russ Cox" <rsc@golang.org> wrote:

> Thanks. I am still internalizing that what GC people mean by 'pin' is not
> what I mean. I changed the CL subject line to
>
> syscall: keep allocated C string live across call to Syscall
>
>
>  --
> 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/d/optout.
>

[brainman, 2014-09-08 23:43:14]

On 2014/09/08 21:00:06, rsc wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=f7d4308ba6fe ***
> 
> syscall: keep allocated C string live across call to Syscall
> 

And why didn't you change windows Syscalls?

Alex

[bradfitz, 2014-09-08 23:51:14]

On Mon, Sep 8, 2014 at 4:43 PM, <alex.brainman@gmail.com> wrote:

> On 2014/09/08 21:00:06, rsc wrote:
>
>> *** Submitted as
>>
> https://code.google.com/p/go/source/detail?r=f7d4308ba6fe ***
>
>  syscall: keep allocated C string live across call to Syscall
>>
>
>
> And why didn't you change windows Syscalls?
>

I imagine because he was trying to fix Solaris, and modified the Perl
script to generate a new zsyscall_solaris_amd64.go, which generated
everything but Windows, because Windows uses its own mksyscall_windows.go
program.  You should make the same edits to that.  It's possible Russ
didn't even notice.

[brainman, 2014-09-09 00:08:09]

> ...  You should make the same edits to that.  ...

Will do. And go.sys too ... Do we need to change ALL (linux and others) go.sys
syscalls too?

Alex

[rsc, 2014-09-09 00:36:32]

On Mon, Sep 8, 2014 at 7:43 PM, <alex.brainman@gmail.com> wrote:

> On 2014/09/08 21:00:06, rsc wrote:
>
>> *** Submitted as
>>
> https://code.google.com/p/go/source/detail?r=f7d4308ba6fe ***
>
>  syscall: keep allocated C string live across call to Syscall
>>
>
> And why didn't you change windows Syscalls?
>

My apologies. I simply forgot there was a Windows-specific system call
generator too.

Russ

[bradfitz, 2014-09-09 00:38:28]

On Mon, Sep 8, 2014 at 5:08 PM, <alex.brainman@gmail.com> wrote:

> ...  You should make the same edits to that.  ...
>>
>
> Will do. And go.sys too ... Do we need to change ALL (linux and others)
> go.sys syscalls too?


I imagine yes.

[brainman, 2014-09-09 00:38:41]

On 2014/09/09 00:36:32, rsc wrote:

This is all well for us, but what about other users who call syscall.Syscall?

Alex

[bradfitz, 2014-09-09 00:39:11]

On Mon, Sep 8, 2014 at 5:38 PM, <alex.brainman@gmail.com> wrote:

> On 2014/09/09 00:36:32, rsc wrote:
>
> This is all well for us, but what about other users who call
> syscall.Syscall?


They get to keep both pieces.

[brainman, 2014-09-09 00:48:49]

On 2014/09/09 00:39:11, bradfitz wrote:
> 
> They get to keep both pieces.

I really hope there is some plan here. If syscall.Syscall interface is broken
and should not be used, we should come up with an alternative.

Alex

[rsc, 2014-09-09 00:51:19]

On Mon, Sep 8, 2014 at 8:48 PM, <alex.brainman@gmail.com> wrote:

> On 2014/09/09 00:39:11, bradfitz wrote:
>
>  They get to keep both pieces.
>>
>
> I really hope there is some plan here. If syscall.Syscall interface is
> broken and should not be used, we should come up with an alternative.
>

Look at the number of changes needed in Brad's CL. It's not every call. If
you write a typed wrapper, everything works fine. It's only the wrappers
that do their own allocations inside the wrapper that are problematic.

Russ

[bradfitz, 2014-09-09 00:52:31]

On Mon, Sep 8, 2014 at 5:48 PM, <alex.brainman@gmail.com> wrote:

> On 2014/09/09 00:39:11, bradfitz wrote:
>
>  They get to keep both pieces.
>>
>
> I really hope there is some plan here. If syscall.Syscall interface is
> broken and should not be used, we should come up with an alternative.
>

Anybody using such a low-level of Go should be reading the release notes
before updating to a newer version, too.

Few people use syscall.Syscall directly, anyway, and it's generally only
(should only be?) advanced users.

[brainman, 2014-09-09 01:16:03]

On 2014/09/09 00:51:19, rsc wrote:
> 
> Look at the number of changes needed in Brad's CL. It's not every call. If
> you write a typed wrapper, everything works fine. It's only the wrappers
> that do their own allocations inside the wrapper that are problematic.
> 

Fair enough. But still it needs to be documented somewhere. We don't want
chasing memory corruption bugs.

On 2014/09/09 00:52:31, bradfitz wrote:
> 
> Few people use syscall.Syscall directly, anyway, and it's generally only
> (should only be?) advanced users.

Unfortunately I cannot say it is true on Windows. Standard library is quite good
for what it does, but anything outside of that you need to call syscall.Syscall.
Every library you need to use is just a dll.

Alex