3 a.m.
On 09/21/21 23:01, Richard W.M. Jones wrote:
On Tue, Sep 21, 2021 at 09:29:39PM +0200, Laszlo Ersek wrote:
> 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>
> Cc: "Richard W.M. Jones" <rjones(a)redhat.com>
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
This time I enabled the golang bindings, reproduced the bug you
reported, applied this patch, reran the tests and verified that it has
been fixed, so:
Tested-by: "Richard W.M. Jones" <rjones(a)redhat.com>
Acked-by: "Richard W.M. Jones" <rjones(a)redhat.com>
Commit e2f8db27d0af.
Thank you!
Laszlo
Rich.
>
> 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
>