Libguestfs - Sep 2021 - [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation

On Mon, Sep 20, 2021 at 07:23:35AM +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,
Doh.
> 
> - 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)

What distro / go version do you see this on, as I can't reproduce
this pointer problem with a standalone demo app ?


> 
> It turns out that a C-language array containing C-language pointers can
> only be allocated in C:
> 
>
https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
> 
> Rewrite the "arg_string_list" and "free_string_list"
functions almost
> entirely in C.
> 
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  generator/golang.ml | 46 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/generator/golang.ml b/generator/golang.ml
> index d328abe4ed08..5db478e8a494 100644
> --- a/generator/golang.ml
> +++ b/generator/golang.ml
> @@ -52,6 +52,39 @@ _go_guestfs_create_flags (unsigned flags)
>  {
>      return guestfs_create_flags (flags);
>  }
> +
> +//
> +// A C-language array of C-language pointers needs to be allocated in C.
> +//
https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
> +//
> +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 +181,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)
>  }
This could be done purely in Go without helper functions

  func array_elem(arr **C.char, idx int) **C.char {
     return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) +
 			(unsafe.Sizeof(arr) * uintptr(idx))))
  }

  func array_size(arr **C.char, len int) C.size_t {
    return C.size_t(unsafe.Sizeof(*arr) * (1 + uintptr(len)))
  }

  func arg_string_list2(xs []string) **C.char {
	var r **C.char
	r = (**C.char)(C.malloc(array_size(len(xs))))
	for i, x := range xs {
		str := array_elem(r, i)
		*str = C.CString(x)
	}
	str := array_elem(r, len(xs))
	*str = nil
	return r
  }

>  
>  func count_string_list (argv **C.char) int {
> @@ -167,11 +199,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))
>  }
This can be done entirely in Go

 func free_string_list (argv **C.char) {
	for i := 0; ; i++ {
		str := array_elem(argv, i)
		if *str == nil {
			break
		}
		C.free(unsafe.Pointer(*str))
	}
	C.free(unsafe.Pointer(argv)) 
}
>  
>  func return_string_list (argv **C.char) []string {
> -- 
> 2.19.1.3.g30247aa5d201
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
> 
Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Sep 20, 2021 at 11:37:02AM +0100, Daniel P. Berrang?
wrote:
> What distro / go version do you see this on, as I can't reproduce
> this pointer problem with a standalone demo app ?
For me this started to happen after upgrading to
golang-bin-1.17-2.fc36.x86_64 in Rawhide.  It also caused this error:

https://gitlab.com/nbdkit/libnbd/-/commit/a550beef2a941fe21d4f834775d73570099dc12a

Fixed by looking at what libvirt-go was doing ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 09/20/21 12:37, Daniel P. Berrang? wrote:
> On Mon, Sep 20, 2021 at 07:23:35AM +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,
> 
> Doh.
In fact I do not think that was an oversight (i.e. that C.free() was
commented out). I didn't try, but I suspect that calling C.free()
triggered the same issue when Rich originally worked on the go bindings,
so C.free() was postponed. And then forgotten. (This is just my
speculation, again.)

> 
>>
>> - 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)
> 
> 
> What distro / go version do you see this on, as I can't reproduce
> this pointer problem with a standalone demo app ?
Stock Fedora 34:

golang-bin-1.16.6-2.fc34.x86_64

It's reproduced reliably by

$ make -C golang V=1 check

in libguestfs.

Interestingly, this kind of "stringlist" is used in two distinct
scenarios in "golang/bindtests/bindtests.go". The first one is when
such
a stringlist is passed to the generated Internal_test() function
[golang/src/libguestfs.org/guestfs/guestfs.go] through the "strlist"
parameter. There are no problems then. But when a similar object is
embedded in the guestfs.OptargsInternal_test argument, that's when
things break.
>> It turns out that a C-language array containing C-language pointers can
>> only be allocated in C:
>>
>>
https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
>>
>> Rewrite the "arg_string_list" and
"free_string_list" functions almost
>> entirely in C.
>>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>>  generator/golang.ml | 46 ++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 37 insertions(+), 9 deletions(-)
>>
>> diff --git a/generator/golang.ml b/generator/golang.ml
>> index d328abe4ed08..5db478e8a494 100644
>> --- a/generator/golang.ml
>> +++ b/generator/golang.ml
>> @@ -52,6 +52,39 @@ _go_guestfs_create_flags (unsigned flags)
>>  {
>>      return guestfs_create_flags (flags);
>>  }
>> +
>> +//
>> +// A C-language array of C-language pointers needs to be allocated in
C.
>> +//
https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
>> +//
>> +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 +181,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)
>>  }
> 
> This could be done purely in Go without helper functions
> 
>   func array_elem(arr **C.char, idx int) **C.char {
>      return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) +
>  			(unsafe.Sizeof(arr) * uintptr(idx))))
>   }
> 
>   func array_size(arr **C.char, len int) C.size_t {
>     return C.size_t(unsafe.Sizeof(*arr) * (1 + uintptr(len)))
>   }
> 
>   func arg_string_list2(xs []string) **C.char {
> 	var r **C.char
> 	r = (**C.char)(C.malloc(array_size(len(xs))))
> 	for i, x := range xs {
> 		str := array_elem(r, i)
> 		*str = C.CString(x)
> 	}
> 	str := array_elem(r, len(xs))
> 	*str = nil
> 	return r
>   }
I'm a total noob in Go and I find the syntax very difficult to read. In
addition to that, I don't have the slightest idea about the Go runtime.
(What I do know however is that the #cgo parser is not a full-blown C
language parser; for example it does not accept "sizeof variable"
without parens, plus see <https://github.com/golang/go/issues/14210> for
another example.) The end result is that I wanted to get out of Go-land
as quickly and as *simply* as possible (meaning the syntax of calling C
code), and then to do the actual business in C. This is the Go binding
of a C library, so I feel this approach is justified.

I concede that the commit message / comments that state that the array
"must" be allocated like this may be a stretch. Note that it's not
without precedent, see this other comment in "generator/golang.ml":

// cgo can't deal with variable argument functions.

Now another comment in <https://github.com/golang/go/issues/14210>, from
Ian Lance Taylor, is:

    That is the way the current implementation works, a consequence of
    the way that cgo rewrites calls to C functions. We can try to do
    better for 1.7, but we aren't going to change this for 1.6.

So the language runtime is even a moving target, and it's not stable
what can be done and what cannot be done in it. I think the code is
justified; perhaps the comments could be relaxed: "it's best to
allocate
arrays like this".

Indeed I don't know *why* Go breaks like that, so I cannot capture that
reason in the commit message. On the other hand, I don't know any other
reasons either why Go does its things the way it does -- and I still
wanted to fix this bug.

I'm happy to pass on this issue to someone else, or (if you can propose
an exact language for the commit message and the code comment) spin a v2
of this patch. Personally I wouldn't like to modify the *code* in the patch.

> 
> 
>>  
>>  func count_string_list (argv **C.char) int {
>> @@ -167,11 +199,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))
>>  }
> 
> This can be done entirely in Go
> 
>  func free_string_list (argv **C.char) {
> 	for i := 0; ; i++ {
> 		str := array_elem(argv, i)
> 		if *str == nil {
> 			break
> 		}
> 		C.free(unsafe.Pointer(*str))
> 	}
> 	C.free(unsafe.Pointer(argv)) 
> }
I guess it "can", but *should* it? In libguestfs? I think (for any
language binding at all, not just Go's), the bindings should be as thin
as possible in the other languages, and most of the business should be in C.

Thanks,
Laszlo

> 
>>  
>>  func return_string_list (argv **C.char) []string {
>> -- 
>> 2.19.1.3.g30247aa5d201
>>
>> _______________________________________________
>> Libguestfs mailing list
>> Libguestfs at redhat.com
>> https://listman.redhat.com/mailman/listinfo/libguestfs
>>
> 
> Regards,
> Daniel
>