search

flier / gohs

GoLang Binding of HyperScan https://www.hyperscan.io/
Other
280 stars 51 forks source link

Fix fatal runtime panic passing invalid pointer #40

Closed aashah closed 2 years ago

aashah commented 2 years ago

The last argument to hs_scan is "void*", a pointer to an abitrary context. "hsScan" incorrectly uses "unsafe.Pointer(h)" when h is not a pointer. This can cause fatal runtime error when Go passes arguments across the Go <-> C boundaries. The runtime error does not happen all the time, as it usually "looks" like a pointer. This makes it difficult to reproduce.

We can fix this by actually passing a real pointer (of our cgo.Handle) and dereferencing the pointer within the callback. I did a quick audit, and think I updated all places where we use the callback.

I did not audit other parts of the API spec that take in a void* arg.

Thanks @adonovan for helping with most of the analysis.

Below is a small snippet of the fatal runtime panic:

runtime: bad pointer in frame github.com/flier/gohs/hyperscan.hsScan.func1 at 0xc0006a5a20: 0x80
fatal error: invalid pointer found on stack

runtime stack:
runtime.throw({0x4a2edb2, 0x5189220})
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/panic.go:1198 +0x71 fp=0x700008aca568 sp=0x700008aca538 pc=0x4037711
runtime.adjustpointers(0x700008aca988, 0x0, 0x5043ae0, {0x5043ae0, 0x5189220})
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/stack.go:617 +0x1d0 fp=0x700008aca5d0 sp=0x700008aca568 pc=0x4050110
runtime.adjustframe(0x700008aca988, 0x700008acaa70)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/stack.go:659 +0xcc fp=0x700008aca680 sp=0x700008aca5d0 pc=0x405022c
runtime.gentraceback(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x7fffffff, 0x4a66b08, 0x0, 0x0)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/traceback.go:350 +0xac3 fp=0x700008aca9f0 sp=0x700008aca680 pc=0x405d523
runtime.copystack(0xc000602d00, 0x8000)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/stack.go:918 +0x293 fp=0x700008acaba0 sp=0x700008aca9f0 pc=0x40509f3
runtime.shrinkstack(0xc000602d00)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/stack.go:1199 +0x126 fp=0x700008acabc0 sp=0x700008acaba0 pc=0x40517c6
runtime.scanstack(0xc000602d00, 0xc000057e98)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/mgcmark.go:719 +0xb2 fp=0x700008acadc0 sp=0x700008acabc0 pc=0x401f772
runtime.markroot.func1()
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/mgcmark.go:232 +0xb1 fp=0x700008acae08 sp=0x700008acadc0 pc=0x401e791
runtime.markroot(0xc000057e98, 0x2d)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/mgcmark.go:205 +0x170 fp=0x700008acae88 sp=0x700008acae08 pc=0x401e550
runtime.gcDrain(0xc000057e98, 0x7)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/mgcmark.go:1013 +0x379 fp=0x700008acaee0 sp=0x700008acae88 pc=0x40203b9
runtime.gcBgMarkWorker.func2()
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/mgc.go:1288 +0x6e fp=0x700008acaf30 sp=0x700008acaee0 pc=0x401d3ce
runtime.systemstack()
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/asm_amd64.s:383 +0x49 fp=0x700008acaf38 sp=0x700008acaf30 pc=0x4068749

...

goroutine 12 [GC assist marking (scan)]:
runtime.systemstack_switch()
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/asm_amd64.s:350 fp=0xc0006a58c8 sp=0xc0006a58c0 pc=0x40686e0
runtime.gcAssistAlloc(0xc000602d00)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/mgcmark.go:447 +0x18b fp=0xc0006a5928 sp=0xc0006a58c8 pc=0x401eeeb
runtime.mallocgc(0x8, 0x48934a0, 0x0)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/malloc.go:959 +0x125 fp=0xc0006a59a8 sp=0xc0006a5928 pc=0x400ed65
runtime.convT64(0x2f811400)
        /usr/local/Cellar/go/1.17.1/libexec/src/runtime/iface.go:364 +0x45 fp=0xc0006a59d0 sp=0xc0006a59a8 pc=0x400cbc5
github.com/flier/gohs/hyperscan.hsScan.func1(0x2f811400, 0xc0003855f0, 0x0, 0x2f809200, 0x52f2760)
        github.com/flier/gohs/hyperscan/internal.go:1049 +0x51 fp=0xc0006a5a38 sp=0xc0006a59d0 pc=0x441b531
github.com/flier/gohs/hyperscan.hsScan(0x0, {0xc000a07500, 0x23, 0x30}, 0xc000600900, 0xc000a07500, 0x0, {0x4933f00, 0xc0000360e8})
        github.com/flier/gohs/hyperscan/internal.go:1049 +0x155 fp=0xc0006a5b18 sp=0xc0006a5a38 pc=0x441b375
github.com/flier/gohs/hyperscan.(*blockScanner).Scan(0xc000779be8, {0xc000a07500, 0x23, 0x1}, 0xc000779be8, 0x30, {0x4933f00, 0xc0000360e8})
        github.com/flier/gohs/hyperscan/runtime.go:351 +0x145 fp=0xc0006a5ba8 sp=0xc0006a5b18 pc=0x4413d65
github.com/flier/gohs/hyperscan.(*blockDatabase).Scan(0xc000011650, {0xc000a07500, 0x23, 0x1}, 0x1, 0x203000, {0x4933f00, 0xc0000360e8})

...
adonovan commented 2 years ago

See https://github.com/golang/go/issues/49633, which suggests a much simpler solution of passing the address of the cgo.Handle through the C code.

flier commented 2 years ago

Well, this is indeed an unthought of problem, thank you very much for your hard work :)