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

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:

https://gitlab.com/nbdkit/libnbd/-/commit/a550beef2a941fe21d4f834775d73570099dc12a

Fixed by looking at what libvirt-go was doing ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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 :|