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

On Mon, Sep 20, 2021 at 05:23:30PM +0200, Laszlo Ersek
wrote:
> On 09/20/21 14:33, Daniel P. Berrang? wrote:
> > On Mon, Sep 20, 2021 at 12:03:51PM +0100, Richard W.M. Jones wrote:
> >> 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:
> > 
> > Hmm, I still cant reproduce the problem that Laszlo is fixing
> > 
> > $ cat str.c
> > 
> > #include <stdio.h>
> > 
> > void foo(char **str) {
> >   for (int i = 0; str[i] != NULL; i++) {
> >     fprintf(stderr, "%d: %s (0x%p)\n", i, str[i], str[i]);
> >   }
> > }
> > 
> > $ cat str.go
> > package main
> > 
> > /*
> > #cgo LDFLAGS: -L/home/berrange/t/lib -lstr
> > 
> > #include <stdlib.h>
> > 
> > extern void foo(char **str);
> > 
> > */
> > import "C"
> > 
> > import (
> > 	"fmt"
> > 	"unsafe"
> > )
> > 
> > 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 arg_string_list1(xs []string) **C.char {
> > 	r := make([]*C.char, 1+len(xs))
> > 	for i, x := range xs {
> > 		r[i] = C.CString(x)
> > 	}
> > 	r[len(xs)] = nil
> > 	return &r[0]
> > }
> > 
> > func arg_string_list2(xs []string) **C.char {
> > 	var r **C.char
> > 	r = (**C.char)(C.malloc(C.size_t(unsafe.Sizeof(*r) * (1 +
uintptr(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 free_string_list(argv **C.char) {
> > 	for i := 0; ; i++ {
> > 		str := (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(argv)) +
> > 			(unsafe.Sizeof(*argv) * uintptr(i))))
> > 		if *str == nil {
> > 			break
> > 		}
> > 		fmt.Printf("%x\n", *str)
> > 		C.free(unsafe.Pointer(*str))
> > 	}
> > }
> > 
> > func bar(str []string) {
> > 	cstr1 := arg_string_list1(str)
> > 	defer free_string_list(cstr1)
> > 	C.foo(cstr1)
> > 	cstr2 := arg_string_list2(str)
> > 	defer free_string_list(cstr2)
> > 	C.foo(cstr2)
> > }
> > 
> > func main() {
> > 	bar([]string{"hello", "world"})
> > }
> > 
> > 
> > My interpretation is that arg_string_list1 impl here should have
> > raised the error that Laszlo reports, but both impls work fine
> 
> Can you create a new structure type, make the C function take the structure
(or a pointer to the structure), and in the structure, make the field have this
type:
> 
>   char * const * str;
> 
> Because this is the scenario where the libguestfs test suite fails
(panics). The libguestfs test suite has a *different* case that does match your
example directly, and *that* case works in the libguestfs test suite flawlessly.
The panic surfaces only in the "char*const* embedded in struct" case.
(I assume "const" makes no difference, but who knows!)
Oh, that makes sense, because you have a Go pointer to the storage for
the struct, and then the 'const *const *str' field is initialized with
a Go pointer returned from the arg_string_list().

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.


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 09/20/21 17:30, Daniel P. Berrang? wrote:
> On Mon, Sep 20, 2021 at 05:23:30PM +0200, Laszlo Ersek wrote:
>> On 09/20/21 14:33, Daniel P. Berrang? wrote:
>>> On Mon, Sep 20, 2021 at 12:03:51PM +0100, Richard W.M. Jones wrote:
>>>> 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:
>>>
>>> Hmm, I still cant reproduce the problem that Laszlo is fixing
>>>
>>> $ cat str.c
>>>
>>> #include <stdio.h>
>>>
>>> void foo(char **str) {
>>>   for (int i = 0; str[i] != NULL; i++) {
>>>     fprintf(stderr, "%d: %s (0x%p)\n", i, str[i],
str[i]);
>>>   }
>>> }
>>>
>>> $ cat str.go
>>> package main
>>>
>>> /*
>>> #cgo LDFLAGS: -L/home/berrange/t/lib -lstr
>>>
>>> #include <stdlib.h>
>>>
>>> extern void foo(char **str);
>>>
>>> */
>>> import "C"
>>>
>>> import (
>>> 	"fmt"
>>> 	"unsafe"
>>> )
>>>
>>> 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 arg_string_list1(xs []string) **C.char {
>>> 	r := make([]*C.char, 1+len(xs))
>>> 	for i, x := range xs {
>>> 		r[i] = C.CString(x)
>>> 	}
>>> 	r[len(xs)] = nil
>>> 	return &r[0]
>>> }
>>>
>>> func arg_string_list2(xs []string) **C.char {
>>> 	var r **C.char
>>> 	r = (**C.char)(C.malloc(C.size_t(unsafe.Sizeof(*r) * (1 +
uintptr(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 free_string_list(argv **C.char) {
>>> 	for i := 0; ; i++ {
>>> 		str := (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(argv)) +
>>> 			(unsafe.Sizeof(*argv) * uintptr(i))))
>>> 		if *str == nil {
>>> 			break
>>> 		}
>>> 		fmt.Printf("%x\n", *str)
>>> 		C.free(unsafe.Pointer(*str))
>>> 	}
>>> }
>>>
>>> func bar(str []string) {
>>> 	cstr1 := arg_string_list1(str)
>>> 	defer free_string_list(cstr1)
>>> 	C.foo(cstr1)
>>> 	cstr2 := arg_string_list2(str)
>>> 	defer free_string_list(cstr2)
>>> 	C.foo(cstr2)
>>> }
>>>
>>> func main() {
>>> 	bar([]string{"hello", "world"})
>>> }
>>>
>>>
>>> My interpretation is that arg_string_list1 impl here should have
>>> raised the error that Laszlo reports, but both impls work fine
>>
>> Can you create a new structure type, make the C function take the
structure (or a pointer to the structure), and in the structure, make the field
have this type:
>>
>>   char * const * str;
>>
>> Because this is the scenario where the libguestfs test suite fails
(panics). The libguestfs test suite has a *different* case that does match your
example directly, and *that* case works in the libguestfs test suite flawlessly.
The panic surfaces only in the "char*const* embedded in struct" case.
(I assume "const" makes no difference, but who knows!)
> 
> Oh, that makes sense, because you have a Go pointer to the storage for
> the struct, and then the 'const *const *str' field is initialized
with
> a Go pointer returned from the arg_string_list().
> 
> 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.
If I take your last paragraph here and work it into the commit message /
comment of this patch, will you accept the code in the patch?

I really don't insist on getting *this* particular patch in. What I'd
like to achieve is a clean "make check" baseline, so I can run the
test
suite regularly, as I get into fixing other libguestfs BZs. I don't
intend to "maintain" Go bindings; I consider this a one-off fix.
I'm
uncomfortable contributing any Go code that's not as "thin" as I
can
possibly manage. (And honestly I think the swaths of "explicit unsafe"
just make things unreadable.)

So I could do two things:

- push patches #1-#3 and drop #4, allowing someone else to fix the issue
described in #4 in more idiomatic Go,

- post a v2 of #4 with updated comments / commit message.

I think a "less idiomatic but technically correct" Go binding is still
an improvement over a panicking Go runtime, but again, if someone can
fix this more idiomatically, I'll *gladly* defer to them.

Rich, what's your take?

Thanks!
Laszlo