gopy,bind: fix storage leaks due to C.Cstring allocations w/ pybindgen

As noted in #186 the use of C.Cstring in conjunction with pybindgen leads to storage leaks. This CL attempts at fixing those by patching the output of pybindgen to call free on the leaked strings.

NOTE, that I plan to look at pybindgen to add an option to it so that it can generate the correct code. In the meantime, I think this change is useful to have since it makes the generated code much more usable.

Fixes #186.

Author: cosnicolaou

SUPPORT_MATRIX.md

@@ -8,6 +8,7 @@ Feature |py2 | py3
 _examples/arrays | yes | yes
 _examples/cgo | yes | yes
 _examples/consts | yes | yes
+_examples/cstrings | yes | yes
 _examples/empty | yes | yes
 _examples/funcs | yes | yes
 _examples/gopygc | yes | yes

_examples/cstrings/cstrings.go

@@ -0,0 +1,48 @@
+// Copyright 2020 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 cstrings
+
+import "strings"
+
+func StringValue(s string, n int) string {
+	return strings.Repeat(s, n)
+}
+
+type StructWithString struct {
+	V string
+	T int
+}
+
+type NestedStructWithString struct {
+	A int
+	S StructWithString
+}
+
+func StringInStruct(s string, n int) StructWithString {
+	return StructWithString{
+		V: strings.Repeat(s, n),
+	}
+}
+
+func StringInNestedStruct(s string, n int) NestedStructWithString {
+	return NestedStructWithString{
+		A: 2,
+		S: StructWithString{
+			V: strings.Repeat(s, n),
+		},
+	}
+}
+
+func StringSlice(s string, n int) []string {
+	return []string{
+		strings.Repeat(s, n),
+	}
+}
+
+func StringMap(s string, n int) map[string]string {
+	return map[string]string{
+		"a": strings.Repeat(s, n),
+	}
+}

_examples/cstrings/test.py

@@ -0,0 +1,89 @@
+# Copyright 2020 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.
+
+# py2/py3 compat
+from __future__ import print_function
+
+import cstrings
+import gc
+import resource
+
+verbose = False
+iterations = 10000
+size = 4096
+
+
+def gofnString():
+    return cstrings.StringValue("a", size)
+
+
+def gofnStruct():
+    s = cstrings.StringInStruct("a", size)
+    return s.V
+
+
+def gofnNestedStruct():
+    s = cstrings.StringInNestedStruct("a", size)
+    return s.S.V
+
+
+def gofnSlice():
+    s = cstrings.StringSlice("a", size)
+    return s[0]
+
+
+def gofnMap():
+    m = cstrings.StringMap("a", size)
+    return m["a"]
+
+
+def print_memory(s):
+    m = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
+    if verbose:
+        print(s, m)
+    return m
+
+
+def _run_fn(fn):
+    memoryvals = []
+    t = [fn() for _ in range(iterations)]
+    memoryvals.append(print_memory(
+        "Memory usage after first list creation is:"))
+
+    t = [fn() for _ in range(iterations)]
+    memoryvals.append(print_memory(
+        "Memory usage after second list creation is:"))
+
+    gc.collect()
+    memoryvals.append(print_memory("Memory usage after GC:"))
+
+    t = [fn() for _ in range(iterations)]
+    memoryvals.append(print_memory(
+        "Memory usage after third list creation is:"))
+
+    gc.collect()
+    memoryvals.append(print_memory("Memory usage after GC:"))
+    return memoryvals
+
+
+for fn in [gofnString, gofnStruct, gofnNestedStruct,  gofnSlice, gofnMap]:
+    alloced = size * iterations
+    a = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
+    pass1 = _run_fn(fn)
+    b = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
+    pass2 = _run_fn(fn)
+    c = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
+    if verbose:
+        print(fn.__name__, pass1)
+        print(fn.__name__, pass2)
+        print(fn.__name__, a, b, c)
+
+    print(fn.__name__,  "leaked: ", (c-b) > (size * iterations))
+
+    # bump up the size of each successive test to ensure that leaks
+    # are not absorbed by previous rss growth.
+    size += 4096
+
+
+print("OK")

bind/gen.go

@@ -316,6 +316,8 @@ build:
 	# use pybindgen to build the %[1]s.c file which are the CPython wrappers to cgo wrappers..
 	# note: pip install pybindgen to get pybindgen if this fails
 	$(PYTHON) build.py
+	# patch storage leaks in pybindgen output
+	go run patch-leaks.go %[1]s.c
 	# build the _%[1]s$(LIBEXT) library that contains the cgo and CPython wrappers
 	# generated %[1]s.py python wrapper imports this c-code package
 	$(GCC) %[1]s.c %[6]s %[1]s_go$(LIBEXT) -o _%[1]s$(LIBEXT) $(CFLAGS) $(LDFLAGS) -fPIC --shared -w
@@ -357,11 +359,91 @@ build:
 	# use pybindgen to build the %[1]s.c file which are the CPython wrappers to cgo wrappers..
 	# note: pip install pybindgen to get pybindgen if this fails
 	$(PYTHON) build.py
+	# patch storage leaks in pybindgen output
+	go run patch-leaks.go %[1]s.c
 	# build the executable
 	- rm %[1]s_go$(LIBEXT)
 	$(GOBUILD) -o py%[1]s
 	
 `
+
+	patchLeaksPreamble = `// patch-leaks.go for post-processing %[1]s.
+// File is generated by gopy. Do not edit.
+// %[2]s
+// +build ignore
+
+package main
+
+import (
+	"bufio"
+	"bytes"
+	"io/ioutil"
+	"log"
+	"os"
+	"strings"
+)
+
+const (
+	cStringLine = "    py_retval = Py_BuildValue((char *) \"s\", retval);"
+)
+var cstringFunctions = []string{
+`
+
+	patchLeaksPostamble = `
+}
+
+func isCString(line string, names []string) bool {
+	for _, cfn := range names {
+		if strings.HasPrefix(line, cfn) {
+			return true
+		}
+	}
+	return false
+}
+
+func patchCString(line string, out *bytes.Buffer) bool {
+	out.WriteString(line)
+	out.Write([]byte{'\n'})
+	switch line {
+	case "}":
+		return false
+	case cStringLine:
+		out.WriteString("    free(retval);\n")
+		return false
+	}
+	return true
+}
+
+func main() {
+	file := os.Args[1]
+	buf, err := ioutil.ReadFile(file)
+	if err != nil {
+		log.Fatal(err)
+	}
+	sc := bufio.NewScanner(bytes.NewBuffer(buf))
+	obuf := &bytes.Buffer{}
+	var cstring bool
+	for sc.Scan() {
+		line := sc.Text()
+		if cstring {
+			cstring = patchCString(line, obuf)
+			continue
+		}
+		cstring = isCString(line, cstringFunctions)
+		obuf.WriteString(line)
+		obuf.Write([]byte{'\n'})
+	}
+
+	if err := sc.Err(); err != nil {
+		log.Fatal(err)
+	}
+
+	err = ioutil.WriteFile(file, obuf.Bytes(), 0644)
+	if err != nil {
+		log.Fatal(err)
+	}
+}
+ `
 )
 
 // thePyGen is the current pyGen which is needed in symbols to lookup
@@ -398,6 +480,7 @@ func GenPyBind(mode BuildMode, odir, outname, cmdstr, vm, mainstr, libext, extra
 
 type pyGen struct {
 	gofile   *printer
+	leakfile *printer
 	pybuild  *printer
 	pywrap   *printer
 	makefile *printer
@@ -445,9 +528,11 @@ func (g *pyGen) genPackageMap() {
 
 func (g *pyGen) genPre() {
 	g.gofile = &printer{buf: new(bytes.Buffer), indentEach: []byte("\t")}
+	g.leakfile = &printer{buf: new(bytes.Buffer), indentEach: []byte("\t")}
 	g.pybuild = &printer{buf: new(bytes.Buffer), indentEach: []byte("\t")}
 	g.makefile = &printer{buf: new(bytes.Buffer), indentEach: []byte("\t")}
 	g.genGoPreamble()
+	g.genLeaksPreamble()
 	g.genPyBuildPreamble()
 	g.genMakefile()
 	oinit, err := os.Create(filepath.Join(g.odir, "__init__.py"))
@@ -468,8 +553,10 @@ func (g *pyGen) genPrintOut(outfn string, pr *printer) {
 func (g *pyGen) genOut() {
 	g.pybuild.Printf("\nmod.generate(open('%v.c', 'w'))\n\n", g.outname)
 	g.gofile.Printf("\n\n")
+	g.genLeaksPostamble()
 	g.makefile.Printf("\n\n")
 	g.genPrintOut(g.outname+".go", g.gofile)
+	g.genPrintOut("patch-leaks.go", g.leakfile)
 	g.genPrintOut("build.py", g.pybuild)
 	g.genPrintOut("Makefile", g.makefile)
 }
@@ -537,6 +624,14 @@ func (g *pyGen) genGoPreamble() {
 	g.gofile.Printf("\n// --- generated code for package: %[1]s below: ---\n\n", g.outname)
 }
 
+func (g *pyGen) genLeaksPreamble() {
+	g.leakfile.Printf(patchLeaksPreamble, g.outname, g.cmdstr)
+}
+
+func (g *pyGen) genLeaksPostamble() {
+	g.leakfile.Printf(patchLeaksPostamble)
+}
+
 func (g *pyGen) genPyBuildPreamble() {
 	g.pybuild.Printf(PyBuildPreamble, g.outname, g.cmdstr)
 }

bind/gen_func.go

@@ -6,9 +6,73 @@ package bind
 
 import (
 	"fmt"
+	"go/types"
 	"strings"
 )
 
+// genPatchLeakFuncName outputs the name of either the function or the class
+// 'getter' represented by its arguments that need to have a memory
+// leak induced by the interaction of pybindgen and gopy.
+// These leaks occur because the cgo bindings use C.CString
+// to allocate a string on the C heap, as in:
+//
+//	return C.CString(cstrings.StringValue(C.GoString(s), int(n)))
+//
+// but the calling pybindgen code then takes a copy of this
+// newly allocated string indirectly by calling Py_BuildValue (see below)
+// without freeing the copy returned by the function or getter method.
+// Arguably pybindgen should provide a means of directly requesting
+// this behaviour (simularly to the transfer_ownership options it provides),
+// but as of now there is no means of doing so. As a work around the
+// output of pybindgen is modified to insert the free in the appropriate
+// functions identified by genPatchLeakFuncName.
+//
+// The patch-leaks.go (see gen.go) file patches these code fragments
+// to insert free(retval) immediately after the call to PyBuildValue.
+// The patch-leaks.go generated file will perform this patch for
+// functions whose names are embedded in it by genPatchLeakFuncName.
+//
+//  .....
+//  retval = cstrings_StringValue(s, n);
+//  py_retval = Py_BuildValue((char *) "s", retval);
+//  .....
+//
+// This patching is required for top level functions that return
+// strings directly and also for getters that return strings that
+// are embedded in maps, structs etc.
+// NOTE that it's ok to output the same function name multiple times (eg.
+// for a shared structs).
+func (g *pyGen) genPatchLeakFuncName(ret *Var, fsym *Func) {
+	if ret == nil || fsym == nil || ret.GoType() == nil {
+		return
+	}
+	g.recurse(ret.GoType(), "_wrap__"+fsym.pkg.Name()+"_", fsym.ID())
+}
+
+func (g *pyGen) recurse(gotype types.Type, prefix, name string) {
+	switch t := gotype.(type) {
+	case *types.Basic:
+		if t.Kind() == types.String {
+			g.leakfile.Printf("    %q,\n", prefix+name)
+		}
+	case *types.Map:
+		g.recurse(t.Elem(), prefix, "Map_"+t.Key().String()+"_string_elem")
+	case *types.Named:
+		o := t.Obj()
+		if o == nil || o.Pkg() == nil {
+			return
+		}
+		g.recurse(t.Underlying(), prefix, o.Pkg().Name()+"_"+o.Name())
+	case *types.Struct:
+		for i := 0; i < t.NumFields(); i++ {
+			f := t.Field(i)
+			g.recurse(f.Type(), prefix, name+"_"+f.Name()+"_Get")
+		}
+	case *types.Slice:
+		g.recurse(t.Elem(), prefix, "Slice_string_elem")
+	}
+}
+
 // genFuncSig generates just the signature for binding
 // returns false if function is not suitable for python
 // binding (e.g., multiple return values)
@@ -110,6 +174,9 @@ func (g *pyGen) genFuncSig(sym *symbol, fsym *Func) bool {
 				ret.Name(),
 			))
 		}
+
+		g.genPatchLeakFuncName(ret, fsym)
+
 		if sret.cpyname == "PyObject*" {
 			g.pybuild.Printf("retval('%s', caller_owns_return=True)", sret.cpyname)
 		} else {