154 lines
5.1 KiB
Diff
154 lines
5.1 KiB
Diff
From e2f8db27d0af5217eb5d0487e0713309559d4489 Mon Sep 17 00:00:00 2001
|
|
From: Laszlo Ersek <lersek@redhat.com>
|
|
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é" <berrange@redhat.com>
|
|
Cc: "Richard W.M. Jones" <rjones@redhat.com>
|
|
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
|
|
Message-Id: <20210921192939.32468-1-lersek@redhat.com>
|
|
Tested-by: "Richard W.M. Jones" <rjones@redhat.com>
|
|
Acked-by: "Richard W.M. Jones" <rjones@redhat.com>
|
|
---
|
|
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:
|
|
+// <https://listman.redhat.com/archives/libguestfs/2021-September/msg00101.html>.
|
|
+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
|
|
|