Skip to content

Commit 76257c7

Browse files
ariel-anieliariel-anieli
committed
runtime/cgo: improve error messages after pointer panic
This CL improves the error messages after panics due to the sharing of an unpinned Go pointer (or a pointer to an unpinned Go pointer) between Go and C. This occurs when it is: 1. returned from Go to C (through cgoCheckResult) 2. passed as argument to a C function (through cgoCheckPointer). An unpinned Go pointer refers to a memory location that might be moved or freed by the garbage collector. Therefore: - change the signature of cgoCheckArg (it does the real work behind cgoCheckResult and cgoCheckPointer) - change the signature of cgoCheckUnknownPointer (called by cgoCheckArg for checking unexpected pointers) - introduce cgoFormatErr (it is called by cgoCheckArg and cgoCheckUnknownPointer to format panic error messages) - update the cgo pointer tests (add new tests, and a field errTextRegexp to the struct ptrTest) - remove a loop variable in TestPointerChecks (similar to CL 711640). 1. cgoCheckResult When an unpinned Go pointer (or a pointer to an unpinned Go pointer) is returned from Go to C, $ cat -n parse.go 1 package main 2 3 import ( 4 "C" 5 ) 6 7 //export foo 8 func foo() map[int]int { 9 return map[int]int{0: 1,} 10 } 11 12 func main() { 13 } $ cat -n parse.py 1 import os 2 from cffi import FFI 3 4 cffi = FFI() 5 cffi.cdef("char* foo();") 6 dll = cffi.dlopen(os.path.abspath("parse.so")) 7 8 dll.foo() 9 cffi.dlclose(dll) $ go build -buildmode=c-shared -o parse.so parse.go $ python parse.py This error shows up at runtime: panic: runtime error: cgo result is unpinned Go pointer or points to unpinned Go pointer goroutine 17 [running, locked to thread]: panic({0x7bf1b12ce460?, 0x3c4f1131c000?}) /mnt/go/src/runtime/panic.go:871 +0x16f runtime.cgoCheckArg(0x7bf1b12cf660, 0x3c4f11318000, 0xd8?, 0x0, {0x7bf1b12a341c, 0x42}) /mnt/go/src/runtime/cgocall.go:628 +0x4c5 runtime.cgoCheckResult({0x7bf1b12cf660, 0x3c4f11318000}) /mnt/go/src/runtime/cgocall.go:795 +0x4b _cgoexp_1c29bd2c0b96_foo(0x7ffd49d02380) /mnt/tmp/parse.go:8 +0x6f runtime.cgocallbackg1(0x7bf1b1299520, 0x7ffd49d02380, 0x0) /mnt/go/src/runtime/cgocall.go:446 +0x289 runtime.cgocallbackg(0x7bf1b1299520, 0x7ffd49d02380, 0x0) /mnt/go/src/runtime/cgocall.go:350 +0x132 runtime.cgocallbackg(0x7bf1b1299520, 0x7ffd49d02380, 0x0) <autogenerated>:1 +0x2b runtime.cgocallback(0x0, 0x0, 0x0) /mnt/go/src/runtime/asm_amd64.s:1101 +0xcd runtime.goexit({}) /mnt/go/src/runtime/asm_amd64.s:1712 +0x1 foo is the faulty Go function; it is not obvious to find it in the stack trace: the stack trace mentions its cgo wrapper (_cgoexp_1c29bd2c0b96_foo). Moreover the error does not say which kind of pointer caused the panic; for instance, a Go map. Retrieve name and file/line of the Go function, as well as the kind of pointer; use them in the error message: panic: runtime error: /mnt/tmp/parse.go:8: result of Go function foo called from cgo is unpinned Go map or points to unpinned Go map goroutine 17 [running, locked to thread]: panic({0x785a83b15cc0?, 0x7ead553a050?}) /mnt/go/src/runtime/panic.go:871 +0x16f runtime.cgoCheckArg(0x785a83b16ec0, 0x7ead55c4060, 0x0?, 0x0, 0x1) /mnt/go/src/runtime/cgocall.go:631 +0x499 runtime.cgoCheckResult({0x785a83b16ec0, 0x7ead55c4060}) /mnt/go/src/runtime/cgocall.go:798 +0x45 _cgoexp_1c29bd2c0b96_foo(0x7ffef3c75230) /mnt/tmp/parse.go:8 +0x6f runtime.cgocallbackg1(0x785a83ae1d80, 0x7ffef3c75230, 0x0) /mnt/go/src/runtime/cgocall.go:446 +0x289 runtime.cgocallbackg(0x785a83ae1d80, 0x7ffef3c75230, 0x0) /mnt/go/src/runtime/cgocall.go:350 +0x132 runtime.cgocallbackg(0x785a83ae1d80, 0x7ffef3c75230, 0x0) <autogenerated>:1 +0x2b runtime.cgocallback(0x0, 0x0, 0x0) /mnt/go/src/runtime/asm_amd64.s:1101 +0xcd runtime.goexit({}) /mnt/go/src/runtime/asm_amd64.s:1712 +0x1 2. cgoCheckPointer When a pointer to an unpinned Go pointer is passed to a C function, 1 package main 2 3 /* 4 #include <stdio.h> 5 void foo(void *bar) {} 6 */ 7 import "C" 8 import "unsafe" 9 10 func main() { 11 m := map[int]int{0: 1,} 12 C.foo(unsafe.Pointer(&m)) 13 } This error shows up at runtime: panic: runtime error: cgo argument has Go pointer to unpinned Go pointer goroutine 1 [running]: main.main.func1(...) /mnt/tmp/cgomap.go:12 main.main() /mnt/tmp/cgomap.go:12 +0x91 exit status 2 Retrieve kind of pointer; use it in the error message. panic: runtime error: argument of cgo function has Go pointer to unpinned Go map goroutine 1 [running]: main.main.func1(...) /mnt/tmp/cgomap.go:12 main.main() /mnt/tmp/cgomap.go:12 +0x9b exit status 2 Link: https://pkg.go.dev/cmd/cgo#hdr-Passing_pointers Suggested-by: Ian Lance Taylor <iant@golang.org> Fixes #75856
1 parent 9f6590f commit 76257c7

File tree

2 files changed

+124
-24
lines changed

2 files changed

+124
-24
lines changed

‎src/cmd/cgo/internal/testerrors/ptr_test.go‎

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"os"
1515
"os/exec"
1616
"path/filepath"
17+
"regexp"
1718
"slices"
1819
"strings"
1920
"sync/atomic"
@@ -24,15 +25,16 @@ var tmp = flag.String("tmp", "", "use `dir` for temporary files and do not clean
2425

2526
// ptrTest is the tests without the boilerplate.
2627
type ptrTest struct {
27-
name string // for reporting
28-
c string // the cgo comment
29-
c1 string // cgo comment forced into non-export cgo file
30-
imports []string // a list of imports
31-
support string // supporting functions
32-
body string // the body of the main function
33-
extra []extra // extra files
34-
fail bool // whether the test should fail
35-
expensive bool // whether the test requires the expensive check
28+
name string // for reporting
29+
c string // the cgo comment
30+
c1 string // cgo comment forced into non-export cgo file
31+
imports []string // a list of imports
32+
support string // supporting functions
33+
body string // the body of the main function
34+
extra []extra // extra files
35+
fail bool // whether the test should fail
36+
expensive bool // whether the test requires the expensive check
37+
errTextRegexp string // error text regexp; if empty, use the pattern `.*unpinned Go.*`
3638
}
3739

3840
type extra struct {
@@ -489,6 +491,27 @@ var ptrTests = []ptrTest{
489491
body: `i := 0; a := &[2]unsafe.Pointer{nil, unsafe.Pointer(&i)}; C.f45(&a[0])`,
490492
fail: true,
491493
},
494+
{
495+
// Passing a Go map as argument to C.
496+
name: "argmap",
497+
c: `void f46(void* p) {}`,
498+
imports: []string{"unsafe"},
499+
body: `m := map[int]int{0: 1,}; C.f46(unsafe.Pointer(&m))`,
500+
fail: true,
501+
errTextRegexp: `.*argument of cgo function has Go pointer to unpinned Go map`,
502+
},
503+
{
504+
// Returning a Go map to C.
505+
name: "retmap",
506+
c: `extern void f47();`,
507+
support: `//export GoMap47
508+
func GoMap47() map[int]int { return map[int]int{0: 1,} }`,
509+
body: `C.f47()`,
510+
c1: `extern void* GoMap47();
511+
void f47() { GoMap47(); }`,
512+
fail: true,
513+
errTextRegexp: `.*result of Go function GoMap47 called from cgo is unpinned Go map or points to unpinned Go map.*`,
514+
},
492515
}
493516

494517
func TestPointerChecks(t *testing.T) {
@@ -519,7 +542,6 @@ func TestPointerChecks(t *testing.T) {
519542
// after testOne finishes.
520543
var pending int32
521544
for _, pt := range ptrTests {
522-
pt := pt
523545
t.Run(pt.name, func(t *testing.T) {
524546
atomic.AddInt32(&pending, +1)
525547
defer func() {
@@ -690,11 +712,17 @@ func testOne(t *testing.T, pt ptrTest, exe, exe2 string) {
690712
}
691713

692714
buf, err := runcmd(cgocheck)
715+
716+
var pattern string = pt.errTextRegexp
717+
if pt.errTextRegexp == "" {
718+
pattern = `.*unpinned Go.*`
719+
}
720+
693721
if pt.fail {
694722
if err == nil {
695723
t.Logf("%s", buf)
696724
t.Fatalf("did not fail as expected")
697-
} else if !bytes.Contains(buf, []byte("Go pointer")) {
725+
} else if ok, _ := regexp.Match(pattern, buf); !ok {
698726
t.Logf("%s", buf)
699727
t.Fatalf("did not print expected error (failed with %v)", err)
700728
}

‎src/runtime/cgocall.go‎

Lines changed: 85 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -591,15 +591,18 @@ func cgoCheckPointer(ptr any, arg any) {
591591
cgoCheckArg(t, ep.data, !t.IsDirectIface(), top, cgoCheckPointerFail)
592592
}
593593

594-
const cgoCheckPointerFail = "cgo argument has Go pointer to unpinned Go pointer"
595-
const cgoResultFail = "cgo result is unpinned Go pointer or points to unpinned Go pointer"
594+
type cgoErrorMsg int
595+
const (
596+
cgoCheckPointerFail cgoErrorMsg = iota
597+
cgoResultFail
598+
)
596599

597600
// cgoCheckArg is the real work of cgoCheckPointer and cgoCheckResult.
598601
// The argument p is either a pointer to the value (of type t), or the value
599602
// itself, depending on indir. The top parameter is whether we are at the top
600603
// level, where Go pointers are allowed. Go pointers to pinned objects are
601604
// allowed as long as they don't reference other unpinned pointers.
602-
func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg string) {
605+
func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg cgoErrorMsg) {
603606
if !t.Pointers() || p == nil {
604607
// If the type has no pointers there is nothing to do.
605608
return
@@ -625,15 +628,15 @@ func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg string) {
625628
// These types contain internal pointers that will
626629
// always be allocated in the Go heap. It's never OK
627630
// to pass them to C.
628-
panic(errorString(msg))
631+
panic(cgoFormatErr(msg, t.Kind()))
629632
case abi.Func:
630633
if indir {
631634
p = *(*unsafe.Pointer)(p)
632635
}
633636
if !cgoIsGoPointer(p) {
634637
return
635638
}
636-
panic(errorString(msg))
639+
panic(cgoFormatErr(msg, t.Kind()))
637640
case abi.Interface:
638641
it := *(**_type)(p)
639642
if it == nil {
@@ -643,14 +646,14 @@ func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg string) {
643646
// constant. A type not known at compile time will be
644647
// in the heap and will not be OK.
645648
if inheap(uintptr(unsafe.Pointer(it))) {
646-
panic(errorString(msg))
649+
panic(cgoFormatErr(msg, t.Kind()))
647650
}
648651
p = *(*unsafe.Pointer)(add(p, goarch.PtrSize))
649652
if !cgoIsGoPointer(p) {
650653
return
651654
}
652655
if !top && !isPinned(p) {
653-
panic(errorString(msg))
656+
panic(cgoFormatErr(msg, t.Kind()))
654657
}
655658
cgoCheckArg(it, p, !it.IsDirectIface(), false, msg)
656659
case abi.Slice:
@@ -661,7 +664,7 @@ func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg string) {
661664
return
662665
}
663666
if !top && !isPinned(p) {
664-
panic(errorString(msg))
667+
panic(cgoFormatErr(msg, t.Kind()))
665668
}
666669
if !st.Elem.Pointers() {
667670
return
@@ -676,7 +679,7 @@ func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg string) {
676679
return
677680
}
678681
if !top && !isPinned(ss.str) {
679-
panic(errorString(msg))
682+
panic(cgoFormatErr(msg, t.Kind()))
680683
}
681684
case abi.Struct:
682685
st := (*structtype)(unsafe.Pointer(t))
@@ -705,7 +708,7 @@ func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg string) {
705708
return
706709
}
707710
if !top && !isPinned(p) {
708-
panic(errorString(msg))
711+
panic(cgoFormatErr(msg, t.Kind()))
709712
}
710713

711714
cgoCheckUnknownPointer(p, msg)
@@ -716,7 +719,7 @@ func cgoCheckArg(t *_type, p unsafe.Pointer, indir, top bool, msg string) {
716719
// memory. It checks whether that Go memory contains any other
717720
// pointer into unpinned Go memory. If it does, we panic.
718721
// The return values are unused but useful to see in panic tracebacks.
719-
func cgoCheckUnknownPointer(p unsafe.Pointer, msg string) (base, i uintptr) {
722+
func cgoCheckUnknownPointer(p unsafe.Pointer, msg cgoErrorMsg) (base, i uintptr) {
720723
if inheap(uintptr(p)) {
721724
b, span, _ := findObject(uintptr(p), 0, 0)
722725
base = b
@@ -731,7 +734,7 @@ func cgoCheckUnknownPointer(p unsafe.Pointer, msg string) (base, i uintptr) {
731734
}
732735
pp := *(*unsafe.Pointer)(unsafe.Pointer(addr))
733736
if cgoIsGoPointer(pp) && !isPinned(pp) {
734-
panic(errorString(msg))
737+
panic(cgoFormatErr(msg, abi.Pointer))
735738
}
736739
}
737740
return
@@ -741,7 +744,7 @@ func cgoCheckUnknownPointer(p unsafe.Pointer, msg string) (base, i uintptr) {
741744
if cgoInRange(p, datap.data, datap.edata) || cgoInRange(p, datap.bss, datap.ebss) {
742745
// We have no way to know the size of the object.
743746
// We have to assume that it might contain a pointer.
744-
panic(errorString(msg))
747+
panic(cgoFormatErr(msg, abi.Pointer))
745748
}
746749
// In the text or noptr sections, we know that the
747750
// pointer does not point to a Go pointer.
@@ -794,3 +797,72 @@ func cgoCheckResult(val any) {
794797
t := ep._type
795798
cgoCheckArg(t, ep.data, !t.IsDirectIface(), false, cgoResultFail)
796799
}
800+
801+
// cgoFormatErr is called by cgoCheckArg and cgoCheckUnknownPointer
802+
// to format panic error messages.
803+
func cgoFormatErr(error cgoErrorMsg, kind abi.Kind) errorString {
804+
var msg, kindname string
805+
var cgoFunction string = "unknown"
806+
var offset int
807+
var buf [20]byte
808+
809+
// We expect one of these abi.Kind from cgoCheckArg
810+
switch kind {
811+
case abi.Chan:
812+
kindname = "channel"
813+
case abi.Func:
814+
kindname = "function"
815+
case abi.Interface:
816+
kindname = "interface"
817+
case abi.Map:
818+
kindname = "map"
819+
case abi.Pointer:
820+
kindname = "pointer"
821+
case abi.Slice:
822+
kindname = "slice"
823+
case abi.String:
824+
kindname = "string"
825+
case abi.Struct:
826+
kindname = "struct"
827+
case abi.UnsafePointer:
828+
kindname = "unsafe pointer"
829+
default:
830+
kindname = "pointer"
831+
}
832+
833+
// The cgo function name might need an offset to be obtained
834+
if error == cgoResultFail {
835+
offset = 21
836+
}
837+
838+
// Relatively to cgoFormatErr, this is the stack frame:
839+
// 0. cgoFormatErr
840+
// 1. cgoCheckArg or cgoCheckUnknownPointer
841+
// 2. cgoCheckPointer or cgoCheckResult
842+
// 3. cgo function
843+
pc, path, line, ok := Caller(3)
844+
if ok && error == cgoResultFail {
845+
function := FuncForPC(pc)
846+
847+
if function != nil {
848+
// Expected format of cgo function name:
849+
// - caller: _cgoexp_3c910ddb72c4_foo
850+
if offset > len(function.Name()) {
851+
cgoFunction = function.Name()
852+
} else {
853+
cgoFunction = function.Name()[offset:]
854+
}
855+
}
856+
}
857+
858+
switch error {
859+
case cgoResultFail:
860+
msg = path + ":" + string(itoa(buf[:], uint64(line)))
861+
msg += ": result of Go function " + cgoFunction + " called from cgo"
862+
msg += " is unpinned Go " + kindname + " or points to unpinned Go " + kindname
863+
case cgoCheckPointerFail:
864+
msg += "argument of cgo function has Go pointer to unpinned Go " + kindname
865+
}
866+
867+
return errorString(msg)
868+
}

0 commit comments

Comments
 (0)