Laszlo Ersek
2021-Sep-20 15:23 UTC
[Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation
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 fineCan 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!)> > $ gcc -fPIC -c -o str.o str.c > $ gcc -shared -o libstr.so str.o > > $ go version > go version go1.17 linux/amd64 > $ go build -o str str.go > $ LD_LIBRARY_PATH=/home/berrange/t/lib ./str > 0: hello (0x0x1d68970) > 1: world (0x0x1d68990) > 0: hello (0x0x1d689d0) > 1: world (0x0x1d689f0) > 1d689d0 > 1d689f0 > 1d68970 > 1d68990 > > > > Is my test scearnio there representative of what the failing test > case is doing ?No, your case represents the one that works fine in libguestfs too. From "golang/bindtests/bindtests.go", generated at commit f47e0bb67254: 41 if err := g.Internal_test ("abc", nil, []string{}, false, 0, 0, "123", "456", []byte{'a', 'b', 'c', '\000', 'a', 'b', 'c'}, &guestfs.OptargsInternal_test{Oint64_is_set: true, Oint64: 1, Ostring_is_set: true, Ostring: "string"}); err != nil { The third argument is such a stringlist, and it works OK. But this one panics due to "Ostringlist": 77 if err := 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{}}); err != nil { The callee (Internal_test()) is in the also-generated source file "golang/src/libguestfs.org/guestfs/guestfs.go". Thanks! Laszlo> Or is perhaps the C function calling back into the Go code ? > > The reason I'm curious is that the current code for arrays here > matches what libvirt-go-module currently uses in some places, so > I'm wondering if that needs fixing too. > > > Regards, > Daniel >
Daniel P. Berrangé
2021-Sep-20 15:30 UTC
[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. 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 :|