Daniel P. Berrangé
2021-Sep-20 12:33 UTC
[Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation
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 $ 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 ? 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 -- |: 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 :|
Richard W.M. Jones
2021-Sep-20 12:51 UTC
[Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation
On Mon, Sep 20, 2021 at 01:33:31PM +0100, Daniel P. Berrang? wrote:> Is my test scearnio there representative of what the failing test > case is doing ? Or is perhaps the C function calling back into > the Go code ?The difference could be that we use the following environment variable when running tests: export GODEBUG=cgocheck=2 Also could you try compiling libnbd after reverting https://gitlab.com/nbdkit/libnbd/-/commit/a550beef2a941fe21d4f834775d73570099dc12a and see if you can reproduce the libnbd error? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
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 >