Re: [Libguestfs] [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.