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/cgo: panic from callbacks does not work with -fsanitize=address/thread #16150

Open
arlolra opened this issue Jun 22, 2016 · 22 comments · Fixed by keroserene/go-webrtc#100
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@arlolra
Copy link

arlolra commented Jun 22, 2016

Version: go1.6 darwin/amd64

==976==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff5fbfefb1 at pc 0x00000573e1bb bp 0x7fff5fbfef10 sp 0x7fff5fbfe6c0
WRITE of size 5 at 0x7fff5fbfefb1 thread T0

As far as I can tell, this is all that's happening:

In go,

msg := C.String("fixme")

In c++,

std::string str(msg);

But maybe I'm missing something obvious.

Have a look,
https://travis-ci.org/keroserene/go-webrtc/jobs/139389584

I've only been able to reproduce this w/ clang.

/cc @dvyukov

@dvyukov
Copy link
Member

dvyukov commented Jun 22, 2016

@kubabrecka @ramosian-glider @kcc
@arlolra can you provide a reproducer?

@arlolra
Copy link
Author

arlolra commented Jun 22, 2016

Checking out HEAD of keroserene/go-webrtc and running,

CGO_CFLAGS="-fsanitize=address" CGO_LDFLAGS="-fsanitize=address" go test -v -race .

seems to produce it ~4% of the time for me. It's transient. But I guess you want an isolated test case ...

@dvyukov
Copy link
Member

dvyukov commented Jun 22, 2016

@arlolra Does it happen without -race? I am surprised it works in some cases, asan and tsan are not supposed to be enabled at the same time. Clang/gcc should not allow that, but it is not detected in such inter-language configuration.

@ianlancetaylor ianlancetaylor changed the title [asan] [cgo] stack-buffer-overflow going from C.String to std::string runtime/cgo: stack-buffer-overflow going from C.String to std::string using -fsanitize=address Jun 22, 2016
@ianlancetaylor
Copy link
Contributor

I don't think -fsanitize=address works reliably with calls between Go and C at present. You should be able to use -fsanitize=memory if you use go build -msan.

@arlolra
Copy link
Author

arlolra commented Jun 22, 2016

Does it happen without -race?

Yes, but anecdotally less often.

Clang/gcc should not allow that, but it is not detected in such inter-language configuration.

"clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'"

I see.

I don't think -fsanitize=address works reliably with calls between Go and C at present.

Ok, good to know.

Thanks all!

@dvyukov
Copy link
Member

dvyukov commented Jun 23, 2016

I've tried to build it on linux with latest clang, and the build reasonably fails with lots of:

/tmp/go-link-682827967/000001.o:(.bss+0x80): multiple definition of `__sanitizer::flag_descriptions'
lib/clang/3.9.0/lib/linux/libclang_rt.asan-x86_64.a(sanitizer_flags.cc.o):projects/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc:36: first defined here

These symbols are in fact defined by asan and race runtimes.
With gcc it works, probably because gcc uses asan runtime in shared library. But nothing good should come up out of this.
Darwin can also use asan runtime in a shared library.

Without -race it seems to work. I run the test 12'000 times without failures.

@arlolra
Copy link
Author

arlolra commented Jun 23, 2016

Thanks for looking into this.

With gcc it works, probably because gcc uses asan runtime in shared library. But nothing good should come up out of this.
Darwin can also use asan runtime in a shared library.

If you look at the matrix I've been using, it's gcc on linux and clang on darwin. Guess that's why I didn't see the duplicate symbols failures.

Without -race it seems to work. I run the test 12'000 times without failures.

Above I said I've only seen this with clang. I should have specified further, clang on darwin. Is that where you ran your tests?

I made this commit yesterday to separate the asan and tsan runs. But, I'm still seeing it on occasion, both locally and in CI.

@arlolra
Copy link
Author

arlolra commented Jun 23, 2016

For example, here's fresh failure from just now https://travis-ci.org/keroserene/go-webrtc/jobs/139634816

@dvyukov
Copy link
Member

dvyukov commented Jun 23, 2016

I've tested on linux. What version of clang are you using?
It looks like somebody leaves garbage in shadow on stack. But I have no idea who.

@arlolra
Copy link
Author

arlolra commented Jun 23, 2016

λ clang -v
Apple LLVM version 7.3.0 (clang-703.0.31)
Target: x86_64-apple-darwin15.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@dvyukov
Copy link
Member

dvyukov commented Jun 23, 2016

@kubabrecka is it the latest clang on mac?

@arlolra
Copy link
Author

arlolra commented Jun 23, 2016

@quentinmit quentinmit added this to the Go1.8 milestone Jun 23, 2016
@quentinmit quentinmit added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 27, 2016

@dvyukov, what's the status here?

@dvyukov dvyukov changed the title runtime/cgo: stack-buffer-overflow going from C.String to std::string using -fsanitize=address runtime/cgo: panic from callbacks does not work with -fsanitize=address/thread Oct 28, 2016
@dvyukov
Copy link
Member

dvyukov commented Oct 28, 2016

I still can't reproduce this, but I think I know why it happens. The program panics from a cgo callback, this effectively does longjmp for C code and longjmp (and other noreturn functions) require special handling for asan/tsan (msan seems to be OK).

I've added throw into unwindm and it fired here:

goroutine 6 [running, locked to thread]:
runtime.throw(0xbbd9cf, 0x7)
    go/src/runtime/panic.go:596 +0x95 fp=0x10c420088bd0 sp=0x10c420088bb0
runtime.unwindm(0x10c420088d87)
    go/src/runtime/cgocall.go:305 +0x4c fp=0x10c420088bf0 sp=0x10c420088bd0
runtime.call32(0x0, 0xbc7fd8, 0x10c4200fa470, 0x800000008)
    go/src/runtime/asm_amd64.s:485 +0x48 fp=0x10c420088c20 sp=0x10c420088bf0
panic(0xb87e80, 0x10c4200f6df0)
    go/src/runtime/panic.go:489 +0x249 fp=0x10c420088cb0 sp=0x10c420088c20
github.com/keroserene/go-webrtc.cgoChannelOnStateChange(0x2)
    gopath/src/github.com/keroserene/go-webrtc/datachannel.go:165 +0xe1 fp=0x10c420088cf8 sp=0x10c420088cb0
github.com/keroserene/go-webrtc._cgoexpwrap_01b08a45bb97_cgoChannelOnStateChange(0x2)
    github.com/keroserene/go-webrtc/_test/_obj_test/_cgo_gotypes.go:732 +0x2b fp=0x10c420088d10 sp=0x10c420088cf8
runtime.call32(0x0, 0x7fff2e5139d8, 0x7fff2e513ab0, 0x8)
    go/src/runtime/asm_amd64.s:485 +0x48 fp=0x10c420088d40 sp=0x10c420088d10
runtime.cgocallbackg1(0x0)
    go/src/runtime/cgocall.go:283 +0x19d fp=0x10c420088db8 sp=0x10c420088d40
runtime.cgocallbackg(0x0)
    go/src/runtime/cgocall.go:170 +0x84 fp=0x10c420088e20 sp=0x10c420088db8
runtime.cgocallback_gofunc(0x4151fa, 0x537753, 0x10c420088ec8, 0xb87e80)
    go/src/runtime/asm_amd64.s:734 +0x74 fp=0x10c420088e40 sp=0x10c420088e20
runtime.asmcgocall(0x537753, 0x10c420088ec8)
    go/src/runtime/asm_amd64.s:581 +0x42 fp=0x10c420088e48 sp=0x10c420088e40
runtime.cgocall(0x537753, 0x10c420088ec8, 0xbc53e1)
    go/src/runtime/cgocall.go:131 +0xfa fp=0x10c420088e88 sp=0x10c420088e48
github.com/keroserene/go-webrtc._Cfunc_CGO_fakeStateChange(0x60060000eec0, 0x6006000003e7)
    github.com/keroserene/go-webrtc/_test/_obj_test/_cgo_gotypes.go:650 +0x45 fp=0x10c420088ec8 sp=0x10c420088e88
github.com/keroserene/go-webrtc.cgoFakeStateChange.func1(0x60060000eec0, 0x10c4000003e7)
    gopath/src/github.com/keroserene/go-webrtc/datachannel.go:196 +0x68 fp=0x10c420088f00 sp=0x10c420088ec8
github.com/keroserene/go-webrtc.cgoFakeStateChange(0x10c420014730, 0x3e7)
    gopath/src/github.com/keroserene/go-webrtc/datachannel.go:196 +0x38 fp=0x10c420088f20 sp=0x10c420088f00
github.com/keroserene/go-webrtc.TestDataStateEnums.func2.1.2.3()
    gopath/src/github.com/keroserene/go-webrtc/datachannel_test.go:89 +0x33 fp=0x10c420088f40 sp=0x10c420088f20
github.com/smartystreets/assertions.ShouldPanic(0xb85e80, 0x10c4200f6dc0, 0x0, 0x0, 0x0, 0x0, 0x0)
    gopath/src/github.com/smartystreets/assertions/panic.go:26 +0xce fp=0x10c420088f88 sp=0x10c420088f40
github.com/smartystreets/goconvey/convey.(*context).So(0x10c420020960, 0xb85e80, 0x10c4200f6dc0, 0xbc7b28, 0x0, 0x0, 0x0)
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:173 +0x5c fp=0x10c420088fd0 sp=0x10c420088f88
github.com/smartystreets/goconvey/convey.So(0xb85e80, 0x10c4200f6dc0, 0xbc7b28, 0x0, 0x0, 0x0)
    gopath/src/github.com/smartystreets/goconvey/convey/doc.go:125 +0x6b fp=0x10c420089018 sp=0x10c420088fd0
github.com/keroserene/go-webrtc.TestDataStateEnums.func2.1.2()
    gopath/src/github.com/keroserene/go-webrtc/datachannel_test.go:90 +0x571 fp=0x10c420089208 sp=0x10c420089018
github.com/smartystreets/goconvey/convey.parseAction.func1(0x1052360, 0x10c420020960)
    gopath/src/github.com/smartystreets/goconvey/convey/discovery.go:80 +0x18 fp=0x10c420089210 sp=0x10c420089208
github.com/smartystreets/goconvey/convey.(*context).conveyInner(0x10c420020960, 0xbc053b, 0x14, 0x10c4200f6ba0)
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:261 +0x162 fp=0x10c420089238 sp=0x10c420089210
github.com/smartystreets/goconvey/convey.(*context).Convey.func1()
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:163 +0x48 fp=0x10c420089268 sp=0x10c420089238
github.com/jtolds/gls._m(0x0, 0x10c420011460)
    gopath/src/github.com/jtolds/gls/stack_tags.go:39 +0x31 fp=0x10c420089288 sp=0x10c420089268
github.com/jtolds/gls.mark2(0x0, 0x10c420011460)
    gopath/src/github.com/jtolds/gls/stack_tags.go:19 +0x35 fp=0x10c4200892a8 sp=0x10c420089288
github.com/jtolds/gls._m(0x2, 0x10c420011460)
    gopath/src/github.com/jtolds/gls/stack_tags.go:41 +0x64 fp=0x10c4200892c8 sp=0x10c4200892a8
github.com/jtolds/gls.markS(0x2, 0x10c420011460)
    gopath/src/github.com/jtolds/gls/stack_tags.go:16 +0x35 fp=0x10c4200892e8 sp=0x10c4200892c8
github.com/jtolds/gls.addStackTag(0x2, 0x10c420011460)
    gopath/src/github.com/jtolds/gls/stack_tags.go:13 +0x3a fp=0x10c420089308 sp=0x10c4200892e8
github.com/jtolds/gls.(*ContextManager).SetValues(0x10c420010460, 0x10c420017020, 0x10c420011460)
    gopath/src/github.com/jtolds/gls/context.go:92 +0x418 fp=0x10c420089480 sp=0x10c420089308
github.com/smartystreets/goconvey/convey.(*context).Convey(0x10c4200207e0, 0x10c420089608, 0x2, 0x2)
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:164 +0x25b fp=0x10c420089540 sp=0x10c420089480
github.com/smartystreets/goconvey/convey.Convey(0x10c420089608, 0x2, 0x2)
    gopath/src/github.com/smartystreets/goconvey/convey/doc.go:77 +0x52 fp=0x10c420089570 sp=0x10c420089540
github.com/keroserene/go-webrtc.TestDataStateEnums.func2.1()
    gopath/src/github.com/keroserene/go-webrtc/datachannel_test.go:91 +0x269 fp=0x10c420089658 sp=0x10c420089570
github.com/smartystreets/goconvey/convey.parseAction.func1(0x1052360, 0x10c4200207e0)
    gopath/src/github.com/smartystreets/goconvey/convey/discovery.go:80 +0x18 fp=0x10c420089660 sp=0x10c420089658
github.com/smartystreets/goconvey/convey.(*context).conveyInner(0x10c4200207e0, 0xbc3049, 0x1f, 0x10c4200f6b10)
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:261 +0x162 fp=0x10c420089688 sp=0x10c420089660
github.com/smartystreets/goconvey/convey.(*context).Convey.func1()
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:163 +0x48 fp=0x10c4200896b8 sp=0x10c420089688
github.com/jtolds/gls._m(0x0, 0x10c420011400)
    gopath/src/github.com/jtolds/gls/stack_tags.go:39 +0x31 fp=0x10c4200896d8 sp=0x10c4200896b8
github.com/jtolds/gls.mark1(0x0, 0x10c420011400)
    gopath/src/github.com/jtolds/gls/stack_tags.go:18 +0x35 fp=0x10c4200896f8 sp=0x10c4200896d8
github.com/jtolds/gls._m(0x1, 0x10c420011400)
    gopath/src/github.com/jtolds/gls/stack_tags.go:41 +0x64 fp=0x10c420089718 sp=0x10c4200896f8
github.com/jtolds/gls.markS(0x1, 0x10c420011400)
    gopath/src/github.com/jtolds/gls/stack_tags.go:16 +0x35 fp=0x10c420089738 sp=0x10c420089718
github.com/jtolds/gls.addStackTag(0x1, 0x10c420011400)
    gopath/src/github.com/jtolds/gls/stack_tags.go:13 +0x3a fp=0x10c420089758 sp=0x10c420089738
github.com/jtolds/gls.(*ContextManager).SetValues(0x10c420010460, 0x10c420016f90, 0x10c420011400)
    gopath/src/github.com/jtolds/gls/context.go:92 +0x418 fp=0x10c4200898d0 sp=0x10c420089758
github.com/smartystreets/goconvey/convey.(*context).Convey(0x10c420020780, 0x10c420089b40, 0x2, 0x2)
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:164 +0x25b fp=0x10c420089990 sp=0x10c4200898d0
github.com/smartystreets/goconvey/convey.Convey(0x10c420089b40, 0x2, 0x2)
    gopath/src/github.com/smartystreets/goconvey/convey/doc.go:77 +0x52 fp=0x10c4200899c0 sp=0x10c420089990
github.com/keroserene/go-webrtc.TestDataStateEnums.func2()
    gopath/src/github.com/keroserene/go-webrtc/datachannel_test.go:107 +0x88f fp=0x10c420089b70 sp=0x10c4200899c0
github.com/smartystreets/goconvey/convey.parseAction.func1(0x1052360, 0x10c420020780)
    gopath/src/github.com/smartystreets/goconvey/convey/discovery.go:80 +0x18 fp=0x10c420089b78 sp=0x10c420089b70
github.com/smartystreets/goconvey/convey.(*context).conveyInner(0x10c420020780, 0xbbe4c0, 0xb, 0x10c4200f6030)
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:261 +0x162 fp=0x10c420089ba0 sp=0x10c420089b78
github.com/smartystreets/goconvey/convey.rootConvey.func1()
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:110 +0xfd fp=0x10c420089bf0 sp=0x10c420089ba0
github.com/jtolds/gls._m(0x0, 0x10c420011020)
    gopath/src/github.com/jtolds/gls/stack_tags.go:39 +0x31 fp=0x10c420089c10 sp=0x10c420089bf0
github.com/jtolds/gls.markS(0x0, 0x10c420011020)
    gopath/src/github.com/jtolds/gls/stack_tags.go:16 +0x35 fp=0x10c420089c30 sp=0x10c420089c10
github.com/jtolds/gls.addStackTag(0x0, 0x10c420011020)
    gopath/src/github.com/jtolds/gls/stack_tags.go:13 +0x3a fp=0x10c420089c50 sp=0x10c420089c30
github.com/jtolds/gls.(*ContextManager).SetValues(0x10c420010460, 0x10c420016c60, 0x10c420011020)
    gopath/src/github.com/jtolds/gls/context.go:92 +0x418 fp=0x10c420089dc8 sp=0x10c420089c50
github.com/smartystreets/goconvey/convey.rootConvey(0x10c420077f08, 0x3, 0x3)
    gopath/src/github.com/smartystreets/goconvey/convey/context.go:113 +0x332 fp=0x10c420089e80 sp=0x10c420089dc8
github.com/smartystreets/goconvey/convey.Convey(0x10c420077f08, 0x3, 0x3)
    gopath/src/github.com/smartystreets/goconvey/convey/doc.go:75 +0x7e fp=0x10c420089eb0 sp=0x10c420089e80
github.com/keroserene/go-webrtc.TestDataStateEnums(0x10c4200ca240)
    gopath/src/github.com/keroserene/go-webrtc/datachannel_test.go:146 +0x1e6 fp=0x10c420089f68 sp=0x10c420089eb0
testing.tRunner(0x10c4200ca240, 0xbc7a30)
    go/src/testing/testing.go:637 +0x85 fp=0x10c420089f90 sp=0x10c420089f68
runtime.goexit()
    go/src/runtime/asm_amd64.s:2164 +0x1 fp=0x10c420089f98 sp=0x10c420089f90
created by testing.(*T).Run
    go/src/testing/testing.go:674 +0x2c4

@dvyukov
Copy link
Member

dvyukov commented Oct 28, 2016

This also makes TestCallbackPanicLoop from misc/cgo crash with -fsanitize=thread.

@dvyukov
Copy link
Member

dvyukov commented Oct 28, 2016

For asan we need to call void __asan_handle_no_return(void) from unwindm. __asan_handle_no_return does not require any preparations/parameters, it simply clears poison from the whole stack of the current thread. We probably don't need to build full support machinery like we do for msan (e.g. adding support to go tool), instead we could detect -fsanitize=address in cmd/cgo like we do for tsan and somehow expose a hook to unwindm. +@ianlancetaylor does it sound feasible?

@ianlancetaylor
Copy link
Contributor

@dvyukov The best idea I've come up with so far is to hook on to the current context parameter that is already passed through crosscall2. Change the C code generated by Package.writeExports in cmd/cgo/out.go to pass down, instead of cgo_ctxt, a pointer to a struct { __SIZE_TYPE__ ctxt; void (*hook)(void); }. Set the hook field based on whether -fsanitize=address was passed to cmd/cgo. This is on the C stack, so all the Go code should continue to treat it as a uintptr. Then in cgocallbackg1 convert ctxt to a pointer to that struct, and handle the ctxt field as it does today. Pass the hook field to unwindm. If unwindm is called with *restore == true, and the hook argument is not nil, then call it. Unfortunately the hook has to be called with an aligned stack, meaning we need yet another function in runtime/asm_GOARCH.s to make the call.

@dvyukov
Copy link
Member

dvyukov commented Oct 28, 2016

The best idea I've come up with so far is to hook on to the current context parameter that is already passed through crosscall2

I was thinking about adding a runtime hook func setUnwindCallback(f uintptr) {unwindCallback = f}. Then cgo will generate an [idempotent] init function that will call runtime.setUnwindCallback(&__asan_handle_no_return). Then unwindm will do: if unwindCallback != 0 {asanCall(f)}.
This looks simpler to me.

Re another asm function. I think we can reuse runtime·racecall, it can call any sanitizer function with up to 4 arguments.

@ianlancetaylor
Copy link
Contributor

OK, your runtime hook sounds good to me.

As far as I can see you can't reuse runtime.racecall because it is only available when building with -race.

@dvyukov
Copy link
Member

dvyukov commented Oct 28, 2016

As far as I can see you can't reuse runtime.racecall because it is only available when building with -race.

I meant to move it to asm_amd64.s and rename to not be race-specific. We will now need that piece of code for race, msan, tsan and asan, duplicating it 4 times does not make sense. And asan/tsan will be detected sort of at runtime in the init callback scheme, so we will need to compile it in unconditionally anyway.

@dvyukov
Copy link
Member

dvyukov commented Oct 28, 2016

Unfortunately I did not have time today to work on this. Next week I am on a conference. Week after next I am in a business trip, not sure if I will have time to work on this.

@dvyukov dvyukov added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 28, 2016
@rsc rsc added this to the Go1.9 milestone Nov 11, 2016
@rsc rsc removed this from the Go1.8 milestone Nov 11, 2016
@bcmills
Copy link
Contributor

bcmills commented Mar 3, 2017

@dvyukov, are you still planning to work on this?

If you're factoring out a function for calling C sanitizer functions, you might consider how that would interact with #18352. (Perhaps one of those refactorings will make the other a bit simpler.)

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 7, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 29, 2017
@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018
uumaro pushed a commit to uumaro/go-webrtc that referenced this issue Nov 30, 2018
This had already been disabled for darwin because of
keroserene#43
golang/go#16150.
Upgrading the Travis dist from trusty to xenial also seems to cause an
error on linux:
keroserene#81 (comment)
arlolra pushed a commit to keroserene/go-webrtc that referenced this issue Nov 30, 2018
This had already been disabled for darwin because of
#43
golang/go#16150.
Upgrading the Travis dist from trusty to xenial also seems to cause an
error on linux:
#81 (comment)
arlolra added a commit to keroserene/go-webrtc that referenced this issue Dec 4, 2018
Until upstream resolves golang/go#16150

Fixes #95
arlolra added a commit to keroserene/go-webrtc that referenced this issue Dec 4, 2018
Until upstream resolves golang/go#16150

Fixes #95
arlolra added a commit to keroserene/go-webrtc that referenced this issue Dec 5, 2018
Until upstream resolves golang/go#16150

Fixes #95
@bcmills bcmills reopened this Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants