From e2f8db27d0af5217eb5d0487e0713309559d4489 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 21 Sep 2021 21:29:39 +0200 Subject: [PATCH] Go bindings: fix "C array of strings" -- char** -- allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-error-cgo-argument-has-go-pointer-to-go-pointer [3] https://listman.redhat.com/archives/libguestfs/2021-September/msg00106.html Cc: "Daniel P. Berrangé" Cc: "Richard W.M. Jones" Signed-off-by: Laszlo Ersek Message-Id: <20210921192939.32468-1-lersek@redhat.com> Tested-by: "Richard W.M. Jones" Acked-by: "Richard W.M. Jones" --- generator/golang.ml | 47 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/generator/golang.ml b/generator/golang.ml index d328abe4e..0d6a92367 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: +// . +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.31.1