New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: check indirect calls for nosplit #12037

Open
rsc opened this Issue Aug 5, 2015 · 6 comments

Comments

Projects
None yet
6 participants
@rsc
Contributor

rsc commented Aug 5, 2015

@rsc rsc added this to the Go1.5Maybe milestone Aug 5, 2015

@rsc rsc modified the milestones: Go1.6Early, Go1.5Maybe Aug 18, 2015

@rsc rsc modified the milestones: Unplanned, Go1.6Early Dec 5, 2015

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned May 11, 2016

@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Oct 21, 2016

@aclements

This comment has been minimized.

Member

aclements commented Jun 6, 2017

Just discussed this briefly with @rsc.

To summarize the problem this issue is about: the nosplit checker can't follow indirect calls, so it's possible to have a nosplit function indirectly call another nosplit function and as a result overflow the stack guard space. This could cause silent stack corruption, which would be very hard to debug.

The options seem to be:

  1. Do nothing. This isn't obviously causing any problems (but maybe it is... who knows?)

  2. Disallow creating function values of nosplit functions, which would make it impossible to call them indirectly (at least via Go code). Also, if you create a function value from a function, the compiler would have to mark that function as "mustsplit" so the linker doesn't auto-nosplit it. This doesn't solve indirect calls from assembly code, but those are probably not a problem.

  3. Don't allow indirect calls from nosplit functions. This probably wouldn't work.

@randall77

This comment has been minimized.

Contributor

randall77 commented Jun 6, 2017

I'd be happy with (2). If the runtime is fine with it, I would expect any other package would be fine with it as well.

@aclements

This comment has been minimized.

Member

aclements commented Jun 6, 2017

I made the following hacky change to find places that violate (2):

--- a/src/cmd/compile/internal/gc/ssa.go
+++ b/src/cmd/compile/internal/gc/ssa.go
@@ -1409,6 +1409,9 @@ func (s *state) expr(n *Node) *ssa.Value {
        case ONAME:
                if n.Class() == PFUNC {
                        // "value" of a function is the address of the function's closure
+                       if n.Name.Defn != nil && n.Name.Defn.Func.Pragma&Nosplit != 0 {
+                               yyerrorl(n.Pos, "cannot use go:nosplit function as function value")
+                       }
                        sym := funcsym(n.Sym).Linksym()
                        aux := s.lookupSymbol(n, &ssa.ExternSymbol{Sym: sym})
                        return s.entryNewValue1A(ssa.OpAddr, types.NewPtr(n.Type), aux, s.sb)

There are four functions. cgocall (as a convenience, we use a function value to select between cgocall and asmcgocall), mstart (which we pass to funcPC), UnlockOSThread (used in a defer), and nilfunc (which we pass to funcPC). For cgocall, we could just expand away the convenience. We could probably just exempt uses with funcPC. And maybe we exempt defer and disallow defer in a nosplit function?

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 14, 2017

Note that (2) also implies marking all exported functions mustsplit.

As for the current problems, I don't see why nilfunc or UnlockOSThread are nosplit. That seems unnecessary to me. (I see why LockOSThread is nosplit, but that's not causing problems.)

I do agree that funcPC should be a special case anyway.

@aclements

This comment has been minimized.

Member

aclements commented Jun 14, 2017

Note that (2) also implies marking all exported functions mustsplit.

That doesn't work so well for exported functions that are nosplit. In fact, (2) might be rather surprising if you, say, try to make a function value of runtime.LockOSThread.

Can we do a hybrid of (2) and (3)? Do (2) within the runtime, but outside of the runtime disallow indirect calls from nosplit functions and mark functions with indirect calls mustsplit? I see exactly one nosplit function in Google's code base outside of runtime/syscall/cgo.

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 14, 2017

Sure. Honestly I'm surprised we recognize nosplit outside of the standard library at all. Maybe that's a mistake.

@aclements aclements modified the milestones: Go1.10, Go1.9 Jul 10, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment