diff --git a/_examples/callbacks/test_callback_rval.py b/_examples/callbacks/test_callback_rval.py new file mode 100644 index 00000000..ff3f8283 --- /dev/null +++ b/_examples/callbacks/test_callback_rval.py @@ -0,0 +1,33 @@ +# Copyright 2015-present, The go-python Authors. All rights reserved. +# Use of this source code is governed by a BSD-style +# license that can be found in the LICENSE file. + +# Regression test for the GIL handling of a Python callback's return value. +# +# When Go calls a Python callback, the generated wrapper takes the GIL, calls +# the callback, and converts the returned object to a Go value. That +# conversion, and the decref of the returned object, must both happen before +# the wrapper releases the GIL again. + +import sys + +import callbacks + +# A callback that keeps returning the same float must not leak a reference to +# it on each call. +X = float("0.25") +before = sys.getrefcount(X) +for _ in range(100): + callbacks.Apply(1.5, lambda x: X) +delta = sys.getrefcount(X) - before +if delta != 0: + raise AssertionError("refcount of the returned float grew by %d over 100 calls" % delta) + +# A float64 callback that returns an int makes the conversion +# (PyFloat_AsDouble) run int.__float__, which allocates a Python float. That +# segfaults if the GIL has already been released. +got = callbacks.Apply(1.5, lambda x: 3) +if got != 3.0: + raise AssertionError("Apply(1.5, lambda x: 3) = %r, want 3.0" % got) + +print("OK") diff --git a/_examples/simple/test_complex_args.py b/_examples/simple/test_complex_args.py new file mode 100644 index 00000000..71f71ddf --- /dev/null +++ b/_examples/simple/test_complex_args.py @@ -0,0 +1,29 @@ +# Copyright 2015-present, The go-python Authors. All rights reserved. +# Use of this source code is governed by a BSD-style +# license that can be found in the LICENSE file. + +# Regression test for the complex64/complex128 GIL-handling fix. +# +# A complex argument is converted with PyComplex_AsCComplex. For an exact +# `complex` that is a plain read of two doubles and works even without the +# GIL, so passing complex values cannot catch the bug. For a float or an int +# it runs Python code (the __complex__ lookup, then __float__/__index__), +# which segfaults if the GIL has already been released for the wrapped Go +# call. Each call below crashed before the fix and returns the right value +# after it. + +import simple as pkg + +cases = [ + ("float", 1.5, 2.5, 4 + 0j), + ("int", 1, 2, 3 + 0j), +] + +for name, a, b, want in cases: + for fn in (pkg.Comp64Add, pkg.Comp128Add): + got = fn(a, b) + # every expected value is exactly representable as a complex64 + if got != want: + raise AssertionError("%s(%s args) = %s, want %s" % (fn.__name__, name, got, want)) + +print("OK") diff --git a/bind/gen_func.go b/bind/gen_func.go index db09390d..0814d93d 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -274,6 +274,47 @@ func isIfaceHandle(gdoc string) (bool, string) { return false, gdoc } +// needsGILForArgMarshal reports whether converting a Python-side argument of +// this symbol's type touches a raw *C.PyObject on the Go side (e.g. +// complex64/complex128 via PyComplex_AsCComplex). Such conversions must run +// while the GIL is still held, not inline in the wrapped call after +// C.PyEval_SaveThread has released it -- touching a PyObject without the GIL +// is undefined behavior. The py2go signature closure (isSignature) is +// excluded: it is invoked later, from inside the wrapped Go call, and already +// reacquires the GIL itself via PyGILState_Ensure/Release around its body. +func needsGILForArgMarshal(sym *symbol) bool { + return sym.cgoname == "*C.PyObject" && !sym.isSignature() && sym.py2go != "" +} + +// argCallExpr returns the Go-side expression used to pass a Python-wrapped +// argument to the wrapped Go call, and whether that expression must be +// evaluated before the GIL is released (see needsGILForArgMarshal). anm is +// the argument's Python-side (pySafeArg) name; isVariadicTail marks the +// trailing "arg" that stands in for a whole variadic slice rather than a +// single marshalled scalar. +// +// This is the single place that decides both questions, in one precedence +// order: ifchandle/interface{} and isSignature never touch a raw PyObject +// via this expression (a callback's wrapper re-acquires the GIL itself, see +// shimABI.callbackArg), so they always report needsGIL=false, regardless of +// the symbol's cgoname. genFuncBody calls it exactly once per argument and +// uses the returned expression (or the premarshalled variable holding it) +// at the call site, so a "_premarshalN" can never be declared under one +// precedence and then left unreferenced under another. +func (g *pyGen) argCallExpr(ifchandle, isVariadicTail bool, anm string, sym *symbol) (expr string, needsGIL bool) { + switch { + case ifchandle && sym.goname == "interface{}": + return fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm), false + case sym.isSignature(): + return g.shim.callbackArg(g, anm, sym), false + case sym.py2go != "": + expr := fmt.Sprintf("%s(%s)%s", sym.py2go, anm, sym.py2goParenEx) + return expr, needsGILForArgMarshal(sym) && !isVariadicTail + default: + return anm, false + } +} + func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { isMethod := (sym != nil) isIface := false @@ -319,6 +360,25 @@ func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { } } + // Decide each argument's call-site expression once, up front, and convert + // any argument whose marshalling touches a raw *C.PyObject now, while the + // GIL is still held -- before saveThread releases it below for the + // duration of the wrapped Go call. The call-argument loop further down + // consumes argExprs as-is, so a premarshalled variable is declared here if + // and only if it is also the expression used at the call site. + argExprs := make([]string, len(args)) + for i, arg := range args { + anm := pySafeArg(arg.Name(), i) + isVariadicTail := fsym.isVariadic && i == len(args)-1 + expr, needsGIL := g.argCallExpr(ifchandle, isVariadicTail, anm, arg.sym) + if needsGIL { + varnm := fmt.Sprintf("_premarshal%d", i) + g.gofile.Printf("%s := %s\n", varnm, expr) + expr = varnm + } + argExprs[i] = expr + } + g.gofile.Print(g.shim.saveThread(!rvIsErr && nres != 2)) if isMethod { @@ -352,19 +412,10 @@ if __err != nil { wrapArgs = append(wrapArgs, "self.handle") } for i, arg := range args { - na := "" anm := pySafeArg(arg.Name(), i) - switch { - case ifchandle && arg.sym.goname == "interface{}": - na = fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm) - case arg.sym.isSignature(): - na = g.shim.callbackArg(g, anm, arg.sym) - case arg.sym.py2go != "": - na = fmt.Sprintf("%s(%s)%s", arg.sym.py2go, anm, arg.sym.py2goParenEx) - default: - na = anm - } - if i == len(args)-1 && fsym.isVariadic { + isVariadicTail := fsym.isVariadic && i == len(args)-1 + na := argExprs[i] + if isVariadicTail { na = na + "..." } callArgs = append(callArgs, na) diff --git a/bind/gen_func_test.go b/bind/gen_func_test.go new file mode 100644 index 00000000..5b527746 --- /dev/null +++ b/bind/gen_func_test.go @@ -0,0 +1,125 @@ +// Copyright 2015-present, The go-python Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package bind + +import "testing" + +// TestArgCallExprPrecedenceMatchesPremarshalDecision guards against the +// failure mode flagged in the PR #401 review of needsGILForArgMarshal: the +// call-argument expression for a Python-wrapped argument used to be decided +// by a switch with a fixed case order (ifchandle&&interface{} > isSignature +// > premarshalled > py2go > default), while a *separate* loop decided, +// independently, whether to emit a "_premarshalN := ..." declaration before +// the GIL is released. Nothing today makes an argument match both an +// earlier switch case and needsGILForArgMarshal, but if it ever did, the +// premarshal declaration would be emitted and never referenced -- +// "_premarshalN declared and not used" -- a compile error surfacing in the +// user's generated package, not in gopy. +// +// argCallExpr is the fix: genFuncBody calls this single function once per +// argument and uses its result both for the premarshal decision and at the +// call site, so a premarshalled variable is declared if and only if it is +// also the expression that gets used. This test exercises that precedence directly, +// including a contrived interface{}/*C.PyObject collision that cannot +// happen with today's type table (interface{}'s cgoname is "*C.char") but +// is exactly the shape the review warned about. +func TestArgCallExprPrecedenceMatchesPremarshalDecision(t *testing.T) { + complex128Sym := &symbol{ + goname: "complex128", + cgoname: "*C.PyObject", + py2go: "complex128FromPyObject", + py2goParenEx: "", + } + + ifaceHandleSym := &symbol{ + goname: "interface{}", + cgoname: "*C.PyObject", // contrived: not true of any symbol today + py2go: "wouldBeMarshalFunc", + py2goParenEx: "", + } + + sigSym := &symbol{ + kind: skSignature, + goname: "func(int) string", + cgoname: "*C.PyObject", + py2go: "func (x int) string { ... }", + py2goParenEx: "", + } + + plainStringSym := &symbol{ + goname: "string", + cgoname: "*C.char", + py2go: "C.GoString", + py2goParenEx: "", + } + + // contrived: a *C.PyObject cgoname with no py2go converter must still + // fall through to the default case, never be premarshalled. + noConvSym := &symbol{ + goname: "int64", + cgoname: "*C.PyObject", + } + + tests := []struct { + name string + ifchandle bool + isVariadic bool + sym *symbol + wantExpr string + wantNeedsGIL bool + }{ + { + name: "complex128 arg is premarshalled", + sym: complex128Sym, + wantExpr: "complex128FromPyObject(x)", + wantNeedsGIL: true, + }, + { + name: "complex128 arg in variadic tail is not premarshalled", + isVariadic: true, + sym: complex128Sym, + wantExpr: "complex128FromPyObject(x)", + wantNeedsGIL: false, + }, + { + name: "ifchandle interface{} wins over premarshalling even if cgoname collides", + ifchandle: true, + sym: ifaceHandleSym, + wantExpr: `gopyh.VarFromHandle((gopyh.CGoHandle)(x), "interface{}")`, + wantNeedsGIL: false, + }, + { + name: "isSignature wins over premarshalling", + sym: sigSym, + wantExpr: "func (x int) string { ... }", + wantNeedsGIL: false, + }, + { + name: "plain string arg is not premarshalled", + sym: plainStringSym, + wantExpr: "C.GoString(x)", + wantNeedsGIL: false, + }, + { + name: "arg without a py2go conversion is passed through as-is", + sym: noConvSym, + wantExpr: "x", + wantNeedsGIL: false, + }, + } + + g := &pyGen{shim: cpythonShim{}} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotExpr, gotNeedsGIL := g.argCallExpr(tt.ifchandle, tt.isVariadic, "x", tt.sym) + if gotExpr != tt.wantExpr { + t.Errorf("argCallExpr() expr = %q, want %q", gotExpr, tt.wantExpr) + } + if gotNeedsGIL != tt.wantNeedsGIL { + t.Errorf("argCallExpr() needsGIL = %v, want %v", gotNeedsGIL, tt.wantNeedsGIL) + } + }) + } +} diff --git a/bind/symbols.go b/bind/symbols.go index c97ff0ad..9ce5ca1b 100644 --- a/bind/symbols.go +++ b/bind/symbols.go @@ -575,6 +575,19 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string) // bstr += fmt.Sprintf("}\n") // } + // PyTuple_New/PyTuple_SetItem below touch raw *C.PyObject values and + // must run with the GIL held. This code is spliced verbatim into a + // caller-generated function body, so the caller is responsible for + // holding the GIL across it -- addSignatureType (its only caller) does + // so already, bracketing this output together with the + // PyObject_CallObject/decref/return-conversion that follows it in the + // same generated closure, under one PyGILState_Ensure/Release. Do not + // re-bracket it here: a self-contained Ensure/Release around just this + // fragment would be redundant with the caller's (nesting is safe, so it + // wouldn't break anything), but it also wouldn't buy anything, since + // the built tuple is only ever used later, by the same caller, which + // must keep holding the GIL regardless. + // // TODO: more efficient to use strings.Builder here.. bstr := fmt.Sprintf("%s := C.PyTuple_New(%d)\n", varnm, sz) for i := 0; i < sz; i++ { @@ -1054,13 +1067,23 @@ func (sym *symtab) addSignatureType(pkg *types.Package, obj types.Object, t type py2g += retstr + "C.PyObject_CallObject(_fun_arg, nil)\n" } py2g += "C.gopy_err_handle()\n" - py2g += "C.PyGILState_Release(_gstate)\n" + // _fcret (if any) is a *C.PyObject -- a new reference returned by + // PyObject_CallObject. Converting it to a Go value (pyObjectToGo, e.g. + // PyLong_AsLongLong/PyBytes_AsString) and decref'ing it both touch a + // raw PyObject and so must happen before PyGILState_Release, not after. + // The converted value is stashed in a local so it can still be returned + // once the GIL is no longer held. if rets.Len() == 1 { cvt, err := sym.pyObjectToGo(ret.Type(), rsym, "_fcret") if err != nil { return err } - py2g += fmt.Sprintf("return %s", cvt) + py2g += fmt.Sprintf("_fcretgo := %s\n", cvt) + py2g += "C.gopy_decref(_fcret)\n" + } + py2g += "C.PyGILState_Release(_gstate)\n" + if rets.Len() == 1 { + py2g += "return _fcretgo" } py2g += "}" diff --git a/bind/symbols_test.go b/bind/symbols_test.go new file mode 100644 index 00000000..2eff7d19 --- /dev/null +++ b/bind/symbols_test.go @@ -0,0 +1,75 @@ +// Copyright 2015-present, The go-python Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package bind + +import ( + "go/token" + "go/types" + "strings" + "testing" +) + +// TestAddSignatureTypeHoldsGILThroughoutClosure guards against the family of +// GIL bugs already fixed once for complex64/complex128 arguments (see +// needsGILForArgMarshal in gen_func.go): generated code that touches a raw +// *C.PyObject must never run outside a PyGILState_Ensure/Release bracket. +// +// addSignatureType's py2go closure is the one place this whole family shows +// up together: it builds a PyTuple from the Go-side arguments (PyTuple_New/ +// SetItem, via buildTuple), invokes the Python callback +// (PyObject_CallObject), and converts the *C.PyObject result back to a Go +// value (pyObjectToGo, e.g. PyBytes_AsString) before decref'ing it. All of +// that touches raw PyObjects and must happen inside the single +// PyGILState_Ensure/Release the closure takes out -- not just the tuple +// build (buildTuple itself does not bracket its own output; it relies on +// its only caller, this closure, to hold the GIL across build, call, and +// return-conversion together). Getting the release point wrong here is not +// hypothetical: _examples/funcs' CallBackRval passes a bool-returning +// callback through exactly this path today. +func TestAddSignatureTypeHoldsGILThroughoutClosure(t *testing.T) { + sig := types.NewSignature(nil, + types.NewTuple(types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int])), + types.NewTuple(types.NewVar(token.NoPos, nil, "", types.Typ[types.String])), + false) + + if err := current.addSignatureType(nil, nil, sig, 0, "sigtest_id", "sigtest"); err != nil { + t.Fatalf("addSignatureType returned error: %v", err) + } + + sym := current.symtype(sig) + if sym == nil { + t.Fatalf("addSignatureType did not register a symbol for %s", current.fullTypeString(sig)) + } + got := sym.py2go + + ensureIdx := strings.Index(got, "C.PyGILState_Ensure()") + lastTupleIdx := strings.LastIndex(got, "C.PyTuple_") + callIdx := strings.Index(got, "C.PyObject_CallObject(") + convIdx := strings.Index(got, "C.PyBytes_AsString(_fcret)") + decrefIdx := strings.Index(got, "C.gopy_decref(_fcret)") + releaseIdx := strings.LastIndex(got, "C.PyGILState_Release(_gstate)") + + for name, idx := range map[string]int{ + "PyGILState_Ensure": ensureIdx, + "PyTuple_*": lastTupleIdx, + "PyObject_CallObject": callIdx, + "PyBytes_AsString": convIdx, + "gopy_decref(_fcret)": decrefIdx, + "final PyGILState_Release": releaseIdx, + } { + if idx == -1 { + t.Fatalf("could not locate expected %s call in generated closure:\n%s", name, got) + } + } + + // Everything that touches a PyObject -- building the tuple, invoking + // the callback, converting and decref'ing the result -- must occur + // strictly between the Ensure and the final Release. + if !(ensureIdx < lastTupleIdx && lastTupleIdx < callIdx && callIdx < convIdx && + convIdx < releaseIdx && decrefIdx < releaseIdx) { + t.Fatalf("generated closure does not hold the GIL across the entire "+ + "build-tuple/call/convert/decref sequence:\n%s", got) + } +} diff --git a/main_test.go b/main_test.go index 9e0339a0..aa995fea 100644 --- a/main_test.go +++ b/main_test.go @@ -1232,6 +1232,84 @@ func TestGilString(t *testing.T) { } } +// TestComplexArgGIL is a regression test for the complex64/128 GIL-handling +// fix. A complex argument is converted with PyComplex_AsCComplex, which must +// run before gopy releases the GIL for the wrapped Go call. Exact `complex` +// values cannot catch a regression, since converting one is a plain struct +// read. The test passes floats and ints instead. Converting those runs +// Python code, so the generated module segfaults if the conversion runs +// after the GIL is released. The return-side conversion +// (complex*GoToPy) takes the GIL itself and is not what this test covers. +func TestComplexArgGIL(t *testing.T) { + t.Parallel() + testExampleScript(t, "TestComplexArgGIL", "_examples/simple", "test_complex_args.py") +} + +// TestCallbackRvalGIL is a regression test for the GIL handling of a Python +// callback's return value. The generated wrapper (see addSignatureType in +// bind/symbols.go) must convert that value to Go and release it before +// PyGILState_Release. The test checks both: +// - a float64 callback that returns an int makes PyFloat_AsDouble run +// int.__float__, which segfaults if the GIL is already released; +// - a callback that keeps returning the same float must not raise that +// float's refcount, which it does if the result is never decref'd. +func TestCallbackRvalGIL(t *testing.T) { + t.Parallel() + testExampleScript(t, "TestCallbackRvalGIL", "_examples/callbacks", "test_callback_rval.py") +} + +// testExampleScript builds the example package at pkgPath, runs script (a +// file in that package's directory) against it with every test backend, and +// expects it to print exactly "OK". +func testExampleScript(t *testing.T, testName, pkgPath, script string) { + backends := []string{"py3"} + for _, be := range backends { + vm, ok := testBackends[be] + if !ok || vm == "" { + t.Logf("Skipped testing backend %s for %s\n", be, testName) + continue + } + t.Run(be, func(t *testing.T) { + t.Parallel() + cwd, _ := os.Getwd() + + workdir, err := os.MkdirTemp("", "gopy-") + if err != nil { + t.Fatalf("could not create workdir: %v", err) + } + defer os.RemoveAll(workdir) + + writeGoMod(t, cwd, workdir) + if err := runGopy([]string{"build", "-vm=" + vm, "-output=" + workdir, "-package-prefix", "", "./" + pkgPath}); err != nil { + t.Fatalf("error building %s: %v", pkgPath, err) + } + + if err := copyCmd(filepath.Join(cwd, pkgPath, script), filepath.Join(workdir, script)); err != nil { + t.Fatalf("error copying %s: %v", script, err) + } + + env := make([]string, len(testEnvironment)) + copy(env, testEnvironment) + env = append(env, fmt.Sprintf("PYTHONPATH=%s", workdir)) + + cmd := exec.Command(vm, "./"+script) + cmd.Env = env + cmd.Dir = workdir + cmd.Stdin = os.Stdin + buf, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("error running python module: err=%v\n%s", err, string(buf)) + } + + got := strings.Replace(string(buf), "\r\n", "\n", -1) + want := "OK\n" + if got != want { + t.Fatalf("got:\n%s\nwant:\n%s", got, want) + } + }) + } +} + func TestPackagePrefix(t *testing.T) { t.Parallel() path := "_examples/package/mypkg"