Skip to content
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: bad context can be passed to cgo traceback function from SWIG calls to _cgo_panic #47995

Closed
prattmic opened this issue Aug 26, 2021 · 10 comments
Assignees
Labels
Milestone

Comments

@prattmic
Copy link
Member

@prattmic prattmic commented Aug 26, 2021

crosscall2 documents an odd case: though it takes a fourth context argument, SWIG (for historical cases) calls it with only three arguments when calling _cgo_panic, leaving the context argument uninitialized.

Normally this is not an issue, as the context would not be used during a panic. However, some rare cases, such as a memory profiling sample in allocation, could trigger a stack trace. At this point, we'll pass the uninitialized context to the cgo traceback function (assuming runtime.SetCgoTraceback was called). This function may then crash or otherwise do something wrong with it.

This is easiest to reproduce by setting runtime.MemProfileRate = 1.

This is broken at least as far back as Go 1.16, but probably much longer.

@prattmic prattmic added this to the Go1.18 milestone Aug 26, 2021
@prattmic prattmic self-assigned this Aug 26, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 26, 2021

Change https://golang.org/cl/345332 mentions this issue: runtime: ignore ctxt argument to crosscall2 calls to _cgo_panic

@prattmic
Copy link
Member Author

@prattmic prattmic commented Aug 26, 2021

My test in https://golang.org/cl/345332 passes on 1.15 and bisection blames http://golang.org/cl/258938. That CL changed crosscall2 of _cgo_panic. Previously, crosscall2 directly called _cgo_panic, which explicitly called cgocallback with a nil ctxt. Afterwards, crosscall2 calls cgocallback directly, passing through the uninitialized ctxt.

cc @aclements

@prattmic
Copy link
Member Author

@prattmic prattmic commented Aug 26, 2021

@gopherbot please open backport issues for 1.16 and 1.17.

This is a serious problem with no workaround besides modifying SWIG, which isn't really feasible.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 26, 2021

Backport issue(s) opened: #47997 (for 1.16), #47998 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 8, 2021

@prattmic Checking in on this issue as we are reviewing backports. Any update on the status?

@prattmic
Copy link
Member Author

@prattmic prattmic commented Sep 8, 2021

I've been on vacation, so the CL hasn't made much progress. Getting back to it now.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 14, 2021

It doesn't help right away, but I'm planning to change SWIG so that instead of using crosscall2, it emits code like this in the generated Go file:

//go:linkname runtime_cgo_panic_internal runtime._cgo_panic_internal
func runtime_cgo_panic_internal(p *byte)

//export cgo_panic__voidtest_ac12e6cc23abb799
func cgo_panic__voidtest_ac12e6cc23abb799(p *byte) {
	runtime_cgo_panic_internal(p)
}

and this in the generated C/C++ file:

extern
#ifdef __cplusplus
  "C"
#endif
  void cgo_panic__voidtest_ac12e6cc23abb799(const char*);
static void _swig_gopanic(const char *p) {
  cgo_panic__voidtest_ac12e6cc23abb799(p);
}

The effect of this will be that future versions of SWIG will not use crosscall2, but they will depend on runtime._cgo_panic_internal.

Does that seem reasonable?

@aclements
Copy link
Member

@aclements aclements commented Sep 15, 2021

@ianlancetaylor , that seems reasonable to me. Is there a reason SWIG can't just call panic from the Go code? Is it just that we really want to be able to use gostringnocopy rather than, say, C.GoString, which does a heap allocation?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 15, 2021

Yeah, I've thought about this more, and I think SWIG should just do it all itself.

My hesitation there is that it's hard to avoid adding this exported function to every SWIG generated file, and since it is exported it will be kept in the binary. The result for a program that uses SWIG for several different libraries is duplication of the panic code. But it may be possible to only emit the exported function if the code actually calls _swig_gopanic. It's not trivial because there are many different ways that SWIG can call that function.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 16, 2021

OK, SWIG versions newer than 4.1.0 no longer call crosscall2.

I'm going to close this issue: the problem is really that SWIG was using an undocumented and unsupported API, and people who encounter the problem described here can now upgrade to a newer version of SWIG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants