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

[Richard W.M. Jones]

On Tue, Sep 21, 2021 at 11:25:23AM +0200, Laszlo Ersek wrote:
> 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?

If it improves the code / test even though it might not be a full fix,
it's worth including IMHO.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html