Re: [Libguestfs] [PATCH v2] Go bindings: fix "C array of strings" -- char** -- allocation

The current "arg_string_list" and "free_string_list" implementations go back to commit b6f01f32606d ("Add Go (language) bindings.", 2013-07-03). There are two problems with them: - "free_string_list" doesn't actually free anything, - at the *first* such g.Internal_test() call site that passes an Ostringlist member inside the Optargs argument, namely: > g.Internal_test ("abc", > string_addr ("def"), > []string{}, > false, > 0, > 0, > "123", > "456", > []byte{'a', 'b', 'c', '\000', 'a', 'b', 'c'}, > &guestfs.OptargsInternal_test{Ostringlist_is_set: true, > Ostringlist: []string{} > } > ) the "golang/run-bindtests" case crashes: > panic: runtime error: cgo argument has Go pointer to Go pointer > > goroutine 1 [running]: > libguestfs.org/guestfs.(*Guestfs).Internal_test.func7(0xc000018180, > 0xadfb60, 0xadfb80, 0xc000010048, 0x0, 0x0, 0x0, 0xae3e10, 0xae3e30, > 0xade3a0, ...) > golang/src/libguestfs.org/guestfs/guestfs.go:6729 +0xa9 > libguestfs.org/guestfs.(*Guestfs).Internal_test(0xc000018180, 0x4ee3a5, > 0x3, 0xc000061be8, 0xc000061af8, 0x0, 0x0, 0xc000061a00, 0x0, 0x0, ...) > golang/src/libguestfs.org/guestfs/guestfs.go:6729 +0x3c9 > main.main() > golang/bindtests/bindtests.go:77 +0x151e > exit status 2 > FAIL run-bindtests (exit status: 1) In Daniel P. Berrangé's words [1], > You're allowed to pass a Go pointer to C via CGo, but the memory that > points to is not allowed to contained further Go pointers. So the struct > fields must strictly use a C pointer. One pattern to solve the problem has been shown on stackoverflow [2]. Thus, rewrite the "arg_string_list" and "free_string_list" functions almost entirely in C, following that example. While this approach is not the most idiomatic Go, as a solution exists without C helper functions [3], it should still be acceptable, at least as an incremental improvement, for letting "golang/run-bindtests" pass. [1] https://listman.redhat.com/archives/libguestfs/2021-September/msg00118.html [2] https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-err... [3] https://listman.redhat.com/archives/libguestfs/2021-September/msg00106.html Cc: "Daniel P. Berrangé" <berrange(a)redhat.com&gt; Cc: "Richard W.M. Jones" <rjones(a)redhat.com&gt; Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt;

Notes: v2: - update commit message [Daniel, Rich] - update code comment [Daniel, Rich] - no code changes - retested generator/golang.ml | 47 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/generator/golang.ml b/generator/golang.ml index d328abe4ed08..0d6a92367f9b 100644 --- a/generator/golang.ml +++ b/generator/golang.ml @@ -52,6 +52,40 @@ _go_guestfs_create_flags (unsigned flags) { return guestfs_create_flags (flags); } + +// Passing a Go pointer to C via CGo is allowed, but the memory that points to +// is not allowed to contain further Go pointers. The helper functions below +// are one way to implement this, although the same can be achieved purely in +// Go as well. See the discussion here: +// <https://listman.redhat.com/archives/libguestfs/2021-September/msg00101.ht...;. +typedef char *pChar; + +pChar *allocStringArray (size_t nmemb) +{ + pChar *array; + + array = malloc (sizeof (pChar) * (nmemb + 1)); + array[nmemb] = NULL; + return array; +} + +void storeString (pChar *array, size_t idx, pChar string) +{ + array[idx] = string; +} + +void freeStringArray (pChar *array) +{ + pChar *position; + pChar string; + + position = array; + while ((string = *position) != NULL) { + free (string); + position++; + } + free (array); +} */ import \"C\" @@ -148,12 +182,11 @@ func (g *Guestfs) Close () *GuestfsError { * C strings and golang []string. */ func arg_string_list (xs []string) **C.char { - r := make ([]*C.char, 1 + len (xs)) + r := C.allocStringArray (C.size_t (len (xs))) for i, x := range xs { - r[i] = C.CString (x) + C.storeString (r, C.size_t (i), C.CString (x)); } - r[len (xs)] = nil - return &r[0] + return (**C.char) (r) } func count_string_list (argv **C.char) int { @@ -167,11 +200,7 @@ func count_string_list (argv **C.char) int { } func free_string_list (argv **C.char) { - for *argv != nil { - //C.free (*argv) - argv = (**C.char) (unsafe.Pointer (uintptr (unsafe.Pointer (argv)) + - unsafe.Sizeof (*argv))) - } + C.freeStringArray ((*C.pChar) (argv)) } func return_string_list (argv **C.char) []string { -- 2.19.1.3.g30247aa5d201